feat(ui): auto-trigger agent when background tasks complete while idle#1571
feat(ui): auto-trigger agent when background tasks complete while idle#1571
Conversation
![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: da76468535
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/kimi_cli/ui/shell/__init__.py
Outdated
| if self.enabled and self._has_pending_llm_notifications(): | ||
| return None |
There was a problem hiding this comment.
wait_for_next returns None as soon as any LLM notification is pending, before it even checks idle_events, so a user input already queued at loop entry is ignored until after an extra auto-triggered run_soul_command finishes. This breaks the stated “user input always takes priority” behavior and can cause an unintended extra model call (cost/latency) whenever a background completion and user submission happen close together (e.g., task finishes during/just after a run and the user types immediately).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds an idle-loop “background completion” watcher to the interactive shell so the agent can automatically re-run when background tasks finish and produce new LLM-facing notifications.
Changes:
- Introduces
_BackgroundCompletionWatcherin the shell loop to wait on either user input or background completion signals and auto-trigger a soul run when appropriate. - Exposes
BackgroundTaskManager.completion_event, setting it when a new terminal task notification is published. - Adds logic to avoid redundant triggers when completions are already consumed during a prior soul run.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/kimi_cli/ui/shell/init.py | Adds idle-loop watcher to auto-trigger the agent on background completion, prioritizing user input. |
| src/kimi_cli/background/manager.py | Adds completion_event and sets it when publishing new terminal notifications. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| done, _ = await asyncio.wait( | ||
| [idle_task, bg_wait_task], | ||
| return_when=asyncio.FIRST_COMPLETED, | ||
| ) | ||
| for t in (idle_task, bg_wait_task): | ||
| if t not in done: | ||
| t.cancel() | ||
| with contextlib.suppress(asyncio.CancelledError): | ||
| await t | ||
|
|
There was a problem hiding this comment.
The “user input always takes priority” guarantee isn’t fully upheld if the background event completes just before user input is enqueued: when bg_wait_task wins, idle_task is cancelled and the queued input will only be processed after the auto-triggered soul run completes. To make input priority deterministic, consider checking idle_events.empty() (or doing a non-blocking get_nowait() fallback) after the background event fires before returning None/triggering a run.
src/kimi_cli/background/manager.py
Outdated
|
|
||
| @property | ||
| def completion_event(self) -> asyncio.Event: | ||
| """Event set when a background task reaches terminal status.""" |
There was a problem hiding this comment.
The completion_event docstring says it is set “when a background task reaches terminal status”, but the event is actually set only when a new terminal notification is published (and won’t be set if the task is terminal but reconcile()/publish_terminal_notifications() hasn’t run yet, or if publishing is deduped). Please adjust the docstring to match the actual semantics so consumers don’t rely on the stronger guarantee.
| """Event set when a background task reaches terminal status.""" | |
| """Event set when a new terminal-status notification is published for a background task. | |
| Note: this is tied to notification publishing, so it may not be set immediately when a | |
| task becomes terminal, and may not fire again for already-terminal tasks if publishing | |
| is deduplicated. | |
| """ |
| notification = self._notifications.publish(event) | ||
| if notification.event.id == event.id: | ||
| published.append(notification.event.id) | ||
| self._completion_event.set() |
There was a problem hiding this comment.
New behavior: _completion_event is set when a terminal notification is newly published. There are already tests for publish_terminal_notifications() and reconcile() in tests/background/test_manager.py, but none assert the event semantics.
Add a test that verifies the event transitions (set on first publish; not set again when the publish is deduped; can be cleared and re-set for a subsequent distinct terminal notification) to prevent regressions.
| if result is None: | ||
| # Background task completed with pending notifications | ||
| logger.info("Background task completed while idle, triggering agent") | ||
| resume_prompt.set() | ||
| await self.run_soul_command( | ||
| "<system-reminder>" | ||
| "Background tasks completed while you were idle." | ||
| "</system-reminder>" | ||
| ) | ||
| console.print() | ||
| if self._exit_after_run: | ||
| console.print("Bye!") | ||
| break | ||
| continue |
There was a problem hiding this comment.
This introduces a new control-flow path where the shell auto-triggers run_soul_command() while idle (including respecting _exit_after_run). There are existing unit tests around Shell.run() behaviors (e.g. tests/ui_and_conv/test_shell_run_placeholders.py), but this new background-completion path isn’t covered.
Consider adding a test that simulates completion_event + a pending LLM notification and asserts exactly one auto-trigger occurs (and that user input is not starved when both signals arrive).
src/kimi_cli/ui/shell/__init__.py
Outdated
| await self.run_soul_command( | ||
| "<system-reminder>" | ||
| "Background tasks completed while you were idle." | ||
| "</system-reminder>" | ||
| ) |
There was a problem hiding this comment.
PR description mentions a fix for “removed acp.TerminalHandle replaced with Client terminal methods”, but this diff doesn’t touch any ACP-related code (and a repo-wide search doesn’t find TerminalHandle). If that fix is no longer part of this PR, consider updating the description to avoid confusion in review/release notes.
| if self.enabled and self._has_pending_llm_notifications(): | ||
| return None | ||
|
|
There was a problem hiding this comment.
wait_for_next() immediately returns None whenever there are pending LLM notifications (even if no new completion_event was observed). If run_soul_command() fails (e.g., LLM not set / provider error), those notifications remain pending and the shell loop will re-enter this branch again immediately, creating a tight retry loop that can spam logs/CPU and make the UI unusable.
Consider gating the auto-trigger on an actual completion signal (e.g., only trigger when completion_event is set), or record/backoff after an auto-trigger attempt (especially on failure) so the loop falls back to waiting for user input instead of immediately re-triggering.
| if self.enabled and self._has_pending_llm_notifications(): | |
| return None |
![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.
- Update completion_event documentation to clarify notification behavior. - Add has_pending_for_sink method to NotificationManager for checking pending deliveries. - Refactor _BackgroundCompletionWatcher to improve handling of background task completions and user input prioritization. - Introduce tests for completion_event and background completion watcher functionality.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30368f28ab
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def has_pending_for_sink(self, sink: str) -> bool: | ||
| """Check whether any notification has a pending delivery for *sink*.""" | ||
| for view in self._store.list_views(): | ||
| sink_state = view.delivery.sinks.get(sink) |
There was a problem hiding this comment.
The shell’s new idle auto-trigger path depends on has_pending_for_sink("llm") (_BackgroundCompletionWatcher.wait_for_next), but this helper reads delivery state without calling recover(). If a prior run crashed after claiming an LLM notification, that sink can stay claimed long enough for the watcher to enter a blocking wait on completion_event; when another component later requeues stale claims, no new publish occurs to set that event, so the agent can stay idle indefinitely until the user submits input. Running recover() before this pending check would prevent missing those recovered notifications.
Useful? React with 👍 / 👎.
Signed-off-by: Kai <me@kaiyi.cool>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4989d32797
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def has_pending_for_sink(self, sink: str) -> bool: | ||
| """Check whether any notification has a pending delivery for *sink*.""" | ||
| for view in self._store.list_views(): | ||
| sink_state = view.delivery.sinks.get(sink) | ||
| if sink_state is not None and sink_state.status == "pending": | ||
| return True |
There was a problem hiding this comment.
has_pending_for_sink() reads delivery state directly but never calls recover(), so notifications that are stuck in claimed (for example after an interrupted/crashed run) are treated as non-pending forever. The new idle auto-trigger path in _BackgroundCompletionWatcher relies on this method, so in that stale-claim state the shell can miss legitimate LLM work and remain idle until manual user input. Calling recover() before scanning (as claim_for_sink() already does) avoids this false-negative state.
Useful? React with 👍 / 👎.
Summary
_BackgroundCompletionWatcherin the shell main loop to detect background task completions while the agent is idle, and automatically trigger a new soul run so the LLM can react to the results.BackgroundTaskManagernow exposes acompletion_eventthat is set whenever a new terminal notification is published._has_pending_llm_notifications) to avoid redundant triggers when notifications were already consumed during a previous soul run.acp.TerminalHandlereplaced withClientterminal methods.Test plan
sleep 5 && echo done), verify the agent auto-triggers when the task completes while idle_exit_after_runis respected after background auto-triggerpytest tests/background/ tests/core/test_notifications.py