Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions assistant/src/daemon/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ import { syncIdentityNameToPlatform } from "../platform/sync-identity.js";
import { buildSystemPrompt } from "../prompts/system-prompt.js";
import { RateLimitProvider } from "../providers/ratelimit.js";
import { getProvider, initializeProviders } from "../providers/registry.js";
import {
registerDefaultWakeResolver,
type WakeTarget,
} from "../runtime/agent-wake.js";
import { buildAssistantEvent } from "../runtime/assistant-event.js";
import { assistantEventHub } from "../runtime/assistant-event-hub.js";
import { DAEMON_INTERNAL_ASSISTANT_ID } from "../runtime/assistant-scope.js";
Expand Down Expand Up @@ -779,6 +783,26 @@ export class DaemonServer {
return { accepted: true };
});

// Install the default resolver for `wakeAgentForOpportunity()` so
// internal subsystems (e.g. the Meet chat-opportunity detector wired
// up in `MeetSessionManager`) can invoke it without having to build
// a `WakeTarget` adapter themselves. The adapter wraps a live
// `Conversation` fetched from the in-memory map / hydrated from the
// DB, exposing only the narrow surface the wake helper needs.
registerDefaultWakeResolver(async (conversationId) => {
try {
const conversation =
await this.getOrCreateConversation(conversationId);
return conversationToWakeTarget(conversation);
Comment on lines +794 to +796
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid creating conversations in default wake resolver

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 👍 / 👎.

} catch (err) {
log.warn(
{ err, conversationId },
"agent-wake default resolver: failed to hydrate conversation",
);
return null;
}
});

// Wire the launchConversation helper to daemon-side state so
// handleSurfaceAction can spawn conversations through it.
registerLaunchConversationDeps({
Expand Down Expand Up @@ -1648,3 +1672,22 @@ function extractConversationId(msg: ServerMessage): string | undefined {
}
return undefined;
}

/**
* Adapt a live {@link Conversation} to the narrow {@link WakeTarget}
* surface expected by `wakeAgentForOpportunity()`. Kept here so the
* runtime-level wake helper stays decoupled from the heavyweight
* conversation class (see `registerDefaultWakeResolver` above).
*/
function conversationToWakeTarget(conversation: Conversation): WakeTarget {
return {
conversationId: conversation.conversationId,
agentLoop: conversation.agentLoop,
getMessages: () => conversation.getMessages(),
pushMessage: (msg) => {
conversation.messages.push(msg);
},
emitToClient: (msg) => conversation.sendToClient(msg),
isProcessing: () => conversation.isProcessing(),
};
}
67 changes: 62 additions & 5 deletions assistant/src/runtime/agent-wake.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,9 @@ export interface WakeResult {
}

/**
* Dependencies injected for testing. Production callers use the defaults
* (which resolve the conversation from the daemon's registry).
* Dependencies injected for testing. Production callers can omit this
* argument entirely and rely on a process-wide default resolver registered
* at daemon startup via {@link registerDefaultWakeResolver}.
*/
export interface WakeDeps {
/** Resolve the wake target for a conversationId. Returns `null` if not found. */
Expand All @@ -84,6 +85,49 @@ export interface WakeDeps {
now?: () => number;
}

// ── 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.
Comment on lines +88 to +100
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Suggested change
// ── 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


let _defaultResolver:
| ((conversationId: string) => Promise<WakeTarget | null>)
| null = null;

/**
* Install the process-wide default resolver. Called once at daemon
* startup (see `DaemonServer.start()`) with an adapter that looks up a
* live {@link Conversation} and wraps it as a {@link WakeTarget}.
*
* Calling this more than once replaces the prior resolver — the daemon
* startup path should call it exactly once, but tests that want to
* exercise the default path can register a mock and reset via
* {@link resetDefaultWakeResolverForTests}.
*/
export function registerDefaultWakeResolver(
resolver: (conversationId: string) => Promise<WakeTarget | null>,
): void {
_defaultResolver = resolver;
}

/**
* Reset the process-wide default resolver. Test-only.
*
* @internal
*/
export function resetDefaultWakeResolverForTests(): void {
_defaultResolver = null;
}

// ── Per-conversation single-flight lock ───────────────────────────────
//
// Simple promise-chain map. When a wake arrives and another run is in
Expand Down Expand Up @@ -183,17 +227,30 @@ function inspectAssistantOutput(
*
* See module-level doc for semantics. Safe to call concurrently; wakes
* are serialized per `conversationId`.
*
* The `deps` argument is optional in production — when omitted, the
* process-wide resolver registered by
* {@link registerDefaultWakeResolver} is used. Tests that want tight
* control over resolution continue to pass explicit deps.
*/
export async function wakeAgentForOpportunity(
opts: WakeOptions,
deps: WakeDeps,
deps?: WakeDeps,
): Promise<WakeResult> {
const { conversationId, hint, source } = opts;
const nowFn = deps.now ?? Date.now;
const resolveTarget = deps?.resolveTarget ?? _defaultResolver;
if (!resolveTarget) {
log.warn(
{ conversationId, source },
"agent-wake: no resolver available (default resolver not registered and no deps passed); skipping",
);
return { invoked: false, producedToolCalls: false };
}
const nowFn = deps?.now ?? Date.now;
const startedAt = nowFn();

return runSingleFlight(conversationId, async () => {
const target = await deps.resolveTarget(conversationId);
const target = await resolveTarget(conversationId);
if (!target) {
log.warn(
{ conversationId, source },
Expand Down
Loading
Loading