runtime: wake the agent loop on internal opportunity (no user message)#25944
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: eb928e4411
ℹ️ 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".
| const ev = event as unknown as ServerMessage; | ||
| buffered.push(ev); |
There was a problem hiding this comment.
In wakeAgentForOpportunity, onEvent currently casts each AgentEvent directly to ServerMessage and buffers it, but AgentLoop.run() emits shapes like text_delta, tool_use, and message_complete { message } that do not match the daemon client protocol (assistant_text_delta, tool_use_start, message_complete metadata, etc.). When a wake produces output, this forwards malformed event payloads to emitToClient, so UI streaming/tool progress can be missing or misinterpreted. Route wake events through the same translation path used in normal turns (or explicitly map supported variants) before emitting.
Useful? React with 👍 / 👎.
| await addMessage( | ||
| conversationId, | ||
| assistantMessage.role, | ||
| JSON.stringify(assistantMessage.content), | ||
| ); |
There was a problem hiding this comment.
This path pushes every post-hint message from updatedHistory into live conversation state, but it persists only one assistant row (assistantMessage) to the DB. For any wake that triggers tools, AgentLoop.run() can produce multiple new messages (assistant tool_use, user tool_result, final assistant text), so persisted transcript/history diverges from in-memory state and loses messages after reload/restart. Persist each appended message (including tool-result user messages and later assistant replies), not just the first assistant message.
Useful? React with 👍 / 👎.
| while (target.isProcessing() && nowFn() < deadline) { | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| } |
There was a problem hiding this comment.
waitUntilIdle stops waiting once the 30s deadline is reached even if target.isProcessing() is still true, and the caller then continues into target.agentLoop.run(). Long-running user turns (e.g., slow tools or approval waits) can exceed this timeout, so the wake can execute a second agent loop concurrently on the same conversation, racing shared history/state because the single-flight lock only serializes wakes, not user turns. On timeout, the wake should abort/defer instead of proceeding while processing is still active.
Useful? React with 👍 / 👎.
|
Follow-up shipped in #26265 — addresses consolidated review feedback (chat concurrency mutex, dispose teardown, wake adapter timeout/ghost prevention, tokenEstimationProvider forwarding, E2E test flakiness). |
1 participant
Summary
Exploration notes
Explored the existing orchestrator path: Conversation.processMessage (daemon/conversation-process.ts) is the primary user-turn entry, which calls Conversation.runAgentLoop (daemon/conversation-agent-loop.ts runAgentLoopImpl), which ultimately invokes conversation.agentLoop.run() from agent/loop.ts. Other call sites that already invoke runAgentLoop directly (subagent/manager.ts, calls/voice-session-bridge.ts) first persist a user message via persistUserMessage — they do persist and emit.
Rather than surgically extracting a shared helper from the user-turn handler (which is 2000+ LOC of accumulated invariants and would risk regressions), agent-wake.ts targets the shared inner core directly: conversation.agentLoop.run(). It builds the run input as [...conversation.messages, internalHint], buffers events during the run, and then decides — based on the produced assistant message — whether to flush to the client and persist. If the assistant produces no tool calls and no visible text, nothing is persisted and nothing is emitted. Tools (wired via conversation.agentLoop's toolExecutor) execute normally when tool_use blocks appear.
This keeps the user-turn handler unchanged and leaves the complex compaction / overflow / convergence machinery out of the wake path (wakes are short, infrequent, and can fall back gracefully).
The WakeTarget interface defines the minimal surface the wake needs (agentLoop.run, getMessages, pushMessage, emitToClient, isProcessing) so the daemon integration (PR 7) just wraps a Conversation instance, and unit tests stub it directly.
Part of plan: meet-phase-2-chat.md (PR 6 of 8)