fix(agent-core-v2): suppress task notifications before loop quiescence on remove - #3720
Conversation
|
d43dfb4 to
5008638
Compare
commit: |
![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: d43dfb42e6
ℹ️ 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".
| managed.closing = true; | ||
| this.onWillCloseEmitter.fire(agent); | ||
| const handle = managed.handle; | ||
| await handle.accessor.get(IAgentTaskService).suppressAllTerminalNotifications(); |
There was a problem hiding this comment.
This fixes observable CLI behavior by preventing background-task notifications from initiating work while an agent is being removed, but the commit contains no .changeset/ entry, so the fix will be omitted from the user-facing release changelog. Add a patch changeset for @moonshot-ai/kimi-code as required by the repository workflow.
AGENTS.md reference: AGENTS.md:L86-L86
Useful? React with 👍 / 👎.
5008638 to
ccbfe69
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccbfe6923d
ℹ️ 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".
| const active = this.list(true); | ||
| await Promise.allSettled( | ||
| active |
There was a problem hiding this comment.
When an in-flight tool registers a detached task, or a foreground task detaches, after this one-time active/detached snapshot and then settles before the post-quiescence stop, it remains unsuppressed. The later stopAllOnExit() scan cannot repair this because list(true) excludes the now-terminal task, so fireTerminalEffects() can still call loop.notify() during remove(), recreating the spurious turn or prolonged quiescence wait this change is intended to prevent. Make teardown suppression sticky for future registrations/detaches until disposal, or apply it atomically when tasks settle during teardown.
Useful? React with 👍 / 👎.
ccbfe69 to
d393414
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3934142d1
ℹ️ 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".
| if (this.exitSuppressionArmed && this.isDetached(entry)) { | ||
| entry.terminalNotificationSuppressed = true; | ||
| } |
There was a problem hiding this comment.
When a detached task enters settleTask() immediately before remove(), it can evaluate this new condition while exitSuppressionArmed is false and then yield in await this.persistLive(entry). Removal can arm suppression during that await, yet settlement resumes at fireTerminalEffects() with terminalNotificationSuppressed unset; the notification builder also does not consult exitSuppressionArmed, so loop.notify() can still initiate work during teardown. The new sticky flag fixes tasks that begin settlement later, but it must also be checked after the await or when terminal effects are fired.
Useful? React with 👍 / 👎.
d393414 to
fe43193
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe43193ea8
ℹ️ 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".
| async suppressAllTerminalNotifications(): Promise<void> { | ||
| if (this.keepAliveOnExit()) return; | ||
| this.exitSuppressionArmed = true; |
There was a problem hiding this comment.
When a detached task finishes just before remove() and notifyAgentTask() is already awaiting its output snapshot, the task's terminal state was recorded before this flag was armed. After the await, buildAgentTaskNotificationContext() checks only the task/ghost terminalNotificationSuppressed field, not exitSuppressionArmed, so it can still call loop.notify() and start a spurious turn or keep loop.settled() pending during teardown. Make the global flag visible to notification construction as well as settlement.
Useful? React with 👍 / 👎.
fe43193 to
ddc8d4b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddc8d4be04
ℹ️ 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".
|
|
||
| private isTerminalNotificationSuppressed(taskId: string): boolean { | ||
| return ( | ||
| this.exitSuppressionArmed || |
There was a problem hiding this comment.
When remove() starts in the microtask between buildAgentTaskNotificationContext() returning and notifyAgentTask() resuming from its await, this new global check has already run, while lifecycleActive() remains true until killSpace(). The caller can therefore still reach loop.notify(), allowing the non-turn-scoped task notification to start a teardown turn or keep loop.settled() pending. Fresh evidence after the earlier review is the remaining post-build await boundary at lines 1101–1106, which has no suppression recheck; consult isTerminalNotificationSuppressed() again immediately before notifying the loop.
Useful? React with 👍 / 👎.
ddc8d4b to
eff4bd4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eff4bd4476
ℹ️ 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".
|
|
||
| async suppressAllTerminalNotifications(): Promise<void> { | ||
| if (this.keepAliveOnExit()) return; | ||
| this.exitSuppressionArmed = true; |
There was a problem hiding this comment.
If a detached task has already reached loop.notify() immediately before remove() arms suppression, its handle remains in pendingNotificationRequests; this method only blocks future notification construction and never drops that queued request. AgentLifecycleService.remove() cancels prompt reservations via pendingPromptIds, but task notifications are loop nudges, and AgentLoopService.settled() continues waiting for undropped nudges, so the notification can still start a turn and delay teardown. Drop the pending task-notification handles when arming suppression.
Useful? React with 👍 / 👎.
eff4bd4 to
f3ca7b1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3ca7b1dac
ℹ️ 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".
| } | ||
|
|
||
| async suppressAllTerminalNotifications(): Promise<void> { | ||
| if (this.keepAliveOnExit()) return; |
There was a problem hiding this comment.
When task.keepAliveOnExit is enabled, this early return leaves terminal notifications unsuppressed even though AgentLifecycleService.remove() calls this method specifically before its quiescence loop. If a retained detached task settles while remove() is awaiting prompt or loop drainage, it can still call loop.notify(), start an unwanted model turn, and delay teardown; keeping the process alive should skip stopping it, not skip teardown-time notification suppression.
Useful? React with 👍 / 👎.
f3ca7b1 to
03606ea
Compare
1 participant
Related Issue
Follow-up to #3717 (stacked on it — merge after that PR). Addresses a Codex review finding on #3717: terminal-notification suppression must happen before awaiting loop quiescence during agent removal.
Problem
#3717 moved
stopAllOnExitbehind the loop/prompt quiescence wait inAgentLifecycleService.remove()so that no new tasks can be registered while tasks are being stopped. ButstopAllOnExitalso suppresses terminal notifications for detached tasks, and delaying it moved that suppression behind the quiescence wait too. A detached task settling naturally in that window was not yet suppressed, sonotifyAgentTask()calledAgentLoopService.notify()— starting a spurious model turn (or keepingloop.settled()pending) in the middle of teardown.What changed
IAgentTaskServicegainssuppressAllTerminalNotifications(), extracted fromstopAllOnExit(which now delegates to it); behavior ofstopAllOnExititself is unchanged.AgentLifecycleService.remove()callssuppressAllTerminalNotifications()before the quiescence loop, so tasks settling mid-teardown stay silent, whilestopAllOnExitstill runs after quiescence to stop everything that remains.FakeTaskServicein the task-tools tests implements the new interface method. Test count unchanged.Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.