meet: wire chat-opportunity detector to agent-wake in session lifecycle#25949
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: d80e24ad46
ℹ️ 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".
| onOpportunity: (hint: string) => { | ||
| void this.deps | ||
| .wakeAgent({ | ||
| conversationId, | ||
| hint, | ||
| source: "meet-chat-opportunity", | ||
| }) |
There was a problem hiding this comment.
The onOpportunity callback unconditionally calls wakeAgent(...) even after leave() has started/finished. leave() only calls chatOpportunityDetector.dispose(), which unsubscribes future events but does not cancel an already in-flight Tier-2 LLM check in MeetChatOpportunityDetector.maybeRunTier2; a verdict that resolves after teardown can still invoke this callback and wake the agent for a meeting that already ended. In that timing window, the assistant can run an extra background turn (and potentially trigger tools) after session shutdown, which violates the intent of the teardown path.
Useful? React with 👍 / 👎.
| const conversation = | ||
| await this.getOrCreateConversation(conversationId); | ||
| return conversationToWakeTarget(conversation); |
There was a problem hiding this comment.
The default resolver uses getOrCreateConversation(conversationId), so a stale/invalid wake target does not resolve to null as wakeAgentForOpportunity expects; it creates a new conversation and proceeds. This is risky in late-callback or deleted-conversation scenarios (for example, an opportunity arriving after meeting teardown), because it can spawn a ghost conversation and run an unintended LLM/tool turn instead of safely skipping. The resolver should resolve an existing conversation (or verify DB existence) and return null when the target is gone.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // ── Process-wide default resolver ──────────────────────────────────── | ||
| // | ||
| // PR 6 shipped `wakeAgentForOpportunity` with a required `deps` argument | ||
| // carrying an explicit `resolveTarget`. PR 7 needs to call the helper | ||
| // from code paths (e.g. `MeetSessionManager.join`) that don't know how | ||
| // to build a `WakeTarget` — the adapter that wraps a live `Conversation` | ||
| // lives in the daemon, not the skill. To avoid importing daemon code | ||
| // into `runtime/agent-wake.ts` (and the skill bundle that wires | ||
| // proactive-chat into the manager), we expose a module-level default | ||
| // resolver that the daemon installs once at startup. Callers that don't | ||
| // pass explicit `deps` fall back to it. Tests that pass explicit deps | ||
| // are unaffected — the default is never consulted when deps are | ||
| // supplied. |
There was a problem hiding this comment.
🔴 Comment narrates PR history, violating assistant/AGENTS.md comment rule
The block comment at assistant/src/runtime/agent-wake.ts:88-100 narrates the codebase's evolution with phrases like "PR 6 shipped wakeAgentForOpportunity with a required deps argument" and "PR 7 needs to call the helper from code paths…". The assistant/AGENTS.md rule states: "Comments should describe the current state of the codebase, not narrate its history. Avoid phrases like 'no longer does X', 'previously used Y', or 'was removed in PR Z'." This comment should be rewritten to describe the current architecture (a module-level default resolver that the daemon installs at startup, allowing callers that don't have access to WakeTarget construction to use the wake helper without explicit deps) without referencing PR numbers or how the design evolved.
| // ── Process-wide default resolver ──────────────────────────────────── | |
| // | |
| // PR 6 shipped `wakeAgentForOpportunity` with a required `deps` argument | |
| // carrying an explicit `resolveTarget`. PR 7 needs to call the helper | |
| // from code paths (e.g. `MeetSessionManager.join`) that don't know how | |
| // to build a `WakeTarget` — the adapter that wraps a live `Conversation` | |
| // lives in the daemon, not the skill. To avoid importing daemon code | |
| // into `runtime/agent-wake.ts` (and the skill bundle that wires | |
| // proactive-chat into the manager), we expose a module-level default | |
| // resolver that the daemon installs once at startup. Callers that don't | |
| // pass explicit `deps` fall back to it. Tests that pass explicit deps | |
| // are unaffected — the default is never consulted when deps are | |
| // supplied. | |
| // ── Process-wide default resolver ──────────────────────────────────── | |
| // | |
| // `wakeAgentForOpportunity` accepts an optional `deps` argument with an | |
| // explicit `resolveTarget`. Code paths that don't know how to build a | |
| // `WakeTarget` (e.g. `MeetSessionManager.join`) omit `deps` and fall | |
| // back to a module-level default resolver that the daemon installs once | |
| // at startup. The adapter that wraps a live `Conversation` as a | |
| // `WakeTarget` lives in the daemon, not here, so the skill bundle never | |
| // imports daemon code. Tests that pass explicit deps are unaffected — | |
| // the default is never consulted when deps are supplied. |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
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
Part of plan: meet-phase-2-chat.md (PR 7 of 8)