From fca2776e61f9263dec2c8f63dc13d95005fce4e3 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Sat, 14 Feb 2026 09:13:15 +0700 Subject: [PATCH] refactor(server): extract agent attention notification policy What changed: - Extracted client/push attention decision rules from websocket-server into a dedicated agent-attention-policy module. - Replaced websocket-server inline branch logic with calls to computeShouldNotifyClient and computeShouldSendPush. - Added deterministic unit tests that cover each notification branch and push suppression rule. Why: - The websocket server carried dense policy branching mixed with transport/session concerns. - Pulling policy into a focused module lowers logic density and makes future changes safer without altering behavior. - Direct unit tests provide confidence for future edits without requiring websocket wiring. Notes for next agent: - This commit intentionally preserves existing behavior and only restructures policy ownership. - If attention behavior needs product changes, update tests in agent-attention-policy.test.ts first. - websocket-server.relay-reconnect.test.ts still covers transport/session reconnection behavior separately. --- .../src/server/agent-attention-policy.test.ts | 239 ++++++++++++++++++ .../src/server/agent-attention-policy.ts | 91 +++++++ .../server/src/server/websocket-server.ts | 94 ++----- 3 files changed, 347 insertions(+), 77 deletions(-) create mode 100644 packages/server/src/server/agent-attention-policy.test.ts create mode 100644 packages/server/src/server/agent-attention-policy.ts diff --git a/packages/server/src/server/agent-attention-policy.test.ts b/packages/server/src/server/agent-attention-policy.test.ts new file mode 100644 index 000000000..e0fdbd8f5 --- /dev/null +++ b/packages/server/src/server/agent-attention-policy.test.ts @@ -0,0 +1,239 @@ +import { describe, expect, it } from "vitest"; +import { + computeShouldNotifyClient, + computeShouldSendPush, + type ClientAttentionState, +} from "./agent-attention-policy.js"; + +function state(overrides: Partial): ClientAttentionState { + return { + deviceType: null, + focusedAgentId: null, + isStale: false, + appVisible: false, + ...overrides, + }; +} + +describe("computeShouldNotifyClient", () => { + it("suppresses notifications when someone is actively focused on the agent", () => { + const activeOnAgent = state({ + deviceType: "web", + focusedAgentId: "agent-1", + isStale: false, + appVisible: true, + }); + const staleMobile = state({ + deviceType: "mobile", + focusedAgentId: null, + isStale: true, + appVisible: false, + }); + + expect( + computeShouldNotifyClient({ + clientState: staleMobile, + allClientStates: [activeOnAgent, staleMobile], + agentId: "agent-1", + }) + ).toBe(false); + }); + + it("notifies unidentified clients by default when agent is not actively focused", () => { + const unknownClient = state({ + deviceType: null, + focusedAgentId: null, + isStale: false, + appVisible: false, + }); + + expect( + computeShouldNotifyClient({ + clientState: unknownClient, + allClientStates: [unknownClient], + agentId: "agent-2", + }) + ).toBe(true); + }); + + it("notifies active visible clients when they are focused on an agent", () => { + const focusedWeb = state({ + deviceType: "web", + focusedAgentId: "agent-2", + isStale: false, + appVisible: true, + }); + + expect( + computeShouldNotifyClient({ + clientState: focusedWeb, + allClientStates: [focusedWeb], + agentId: "agent-3", + }) + ).toBe(true); + }); + + it("suppresses active clients that are not focused on an agent", () => { + const activeButNotFocused = state({ + deviceType: "web", + focusedAgentId: null, + isStale: false, + appVisible: true, + }); + + expect( + computeShouldNotifyClient({ + clientState: activeButNotFocused, + allClientStates: [activeButNotFocused], + agentId: "agent-4", + }) + ).toBe(false); + }); + + it("suppresses stale mobile notifications when an active web client exists", () => { + const staleMobile = state({ + deviceType: "mobile", + focusedAgentId: null, + isStale: true, + appVisible: false, + }); + const activeWeb = state({ + deviceType: "web", + focusedAgentId: null, + isStale: false, + appVisible: true, + }); + + expect( + computeShouldNotifyClient({ + clientState: staleMobile, + allClientStates: [staleMobile, activeWeb], + agentId: "agent-5", + }) + ).toBe(false); + }); + + it("allows stale mobile notifications when no active web client exists", () => { + const staleMobile = state({ + deviceType: "mobile", + focusedAgentId: null, + isStale: true, + appVisible: false, + }); + const staleWeb = state({ + deviceType: "web", + focusedAgentId: null, + isStale: true, + appVisible: false, + }); + + expect( + computeShouldNotifyClient({ + clientState: staleMobile, + allClientStates: [staleMobile, staleWeb], + agentId: "agent-6", + }) + ).toBe(true); + }); + + it("suppresses stale web notifications when a mobile client is also present", () => { + const staleWeb = state({ + deviceType: "web", + focusedAgentId: null, + isStale: true, + appVisible: false, + }); + const staleMobile = state({ + deviceType: "mobile", + focusedAgentId: null, + isStale: true, + appVisible: false, + }); + + expect( + computeShouldNotifyClient({ + clientState: staleWeb, + allClientStates: [staleWeb, staleMobile], + agentId: "agent-7", + }) + ).toBe(false); + }); + + it("allows stale web notifications when there are no mobile or unidentified clients", () => { + const staleWeb = state({ + deviceType: "web", + focusedAgentId: null, + isStale: true, + appVisible: false, + }); + + expect( + computeShouldNotifyClient({ + clientState: staleWeb, + allClientStates: [staleWeb], + agentId: "agent-8", + }) + ).toBe(true); + }); +}); + +describe("computeShouldSendPush", () => { + it("never sends push for error attention events", () => { + expect( + computeShouldSendPush({ + reason: "error", + allClientStates: [], + }) + ).toBe(false); + }); + + it("suppresses push when any active web client exists", () => { + expect( + computeShouldSendPush({ + reason: "finished", + allClientStates: [ + state({ + deviceType: "web", + isStale: false, + appVisible: true, + }), + ], + }) + ).toBe(false); + }); + + it("suppresses push when a mobile app is actively visible", () => { + expect( + computeShouldSendPush({ + reason: "permission", + allClientStates: [ + state({ + deviceType: "mobile", + isStale: false, + appVisible: true, + }), + ], + }) + ).toBe(false); + }); + + it("sends push when no active web client or foreground mobile client exists", () => { + expect( + computeShouldSendPush({ + reason: "finished", + allClientStates: [ + state({ + deviceType: "mobile", + isStale: true, + appVisible: false, + }), + state({ + deviceType: "web", + isStale: true, + appVisible: false, + }), + ], + }) + ).toBe(true); + }); +}); diff --git a/packages/server/src/server/agent-attention-policy.ts b/packages/server/src/server/agent-attention-policy.ts new file mode 100644 index 000000000..bf7c40f9f --- /dev/null +++ b/packages/server/src/server/agent-attention-policy.ts @@ -0,0 +1,91 @@ +export type ClientAttentionState = { + deviceType: "web" | "mobile" | null; + focusedAgentId: string | null; + isStale: boolean; + appVisible: boolean; +}; + +export type AgentAttentionReason = "finished" | "error" | "permission"; + +type ComputeClientNotificationInput = { + clientState: ClientAttentionState; + allClientStates: ClientAttentionState[]; + agentId: string; +}; + +type ComputePushNotificationInput = { + reason: AgentAttentionReason; + allClientStates: ClientAttentionState[]; +}; + +function hasActiveClientOnAgent( + allClientStates: ClientAttentionState[], + agentId: string +): boolean { + return allClientStates.some( + (state) => state.focusedAgentId === agentId && state.appVisible && !state.isStale + ); +} + +function hasActiveWebClient(allClientStates: ClientAttentionState[]): boolean { + return allClientStates.some((state) => state.deviceType === "web" && !state.isStale); +} + +function hasOtherCompetingClient( + clientState: ClientAttentionState, + allClientStates: ClientAttentionState[] +): boolean { + return allClientStates.some( + (state) => + state !== clientState && (state.deviceType === "mobile" || state.deviceType === null) + ); +} + +function hasActiveForegroundMobileClient(allClientStates: ClientAttentionState[]): boolean { + return allClientStates.some( + (state) => state.deviceType === "mobile" && state.appVisible && !state.isStale + ); +} + +export function computeShouldNotifyClient({ + clientState, + allClientStates, + agentId, +}: ComputeClientNotificationInput): boolean { + if (hasActiveClientOnAgent(allClientStates, agentId)) { + return false; + } + + if (clientState.deviceType === null) { + return true; + } + + if (!clientState.isStale && clientState.appVisible && clientState.focusedAgentId !== null) { + return true; + } + + if (!clientState.isStale) { + return false; + } + + if (clientState.deviceType === "mobile") { + return !hasActiveWebClient(allClientStates); + } + + if (clientState.deviceType === "web") { + return !hasOtherCompetingClient(clientState, allClientStates); + } + + return true; +} + +export function computeShouldSendPush({ + reason, + allClientStates, +}: ComputePushNotificationInput): boolean { + if (reason === "error") { + return false; + } + + return !hasActiveWebClient(allClientStates) && !hasActiveForegroundMobileClient(allClientStates); +} diff --git a/packages/server/src/server/websocket-server.ts b/packages/server/src/server/websocket-server.ts index df165f530..94e6a4b51 100644 --- a/packages/server/src/server/websocket-server.ts +++ b/packages/server/src/server/websocket-server.ts @@ -34,6 +34,11 @@ import type { VoiceMcpStdioConfig, VoiceSpeakHandler, } from "./voice-types.js"; +import { + computeShouldNotifyClient, + computeShouldSendPush, + type ClientAttentionState, +} from "./agent-attention-policy.js"; export type AgentMcpTransportFactory = () => Promise; export type ExternalSocketMetadata = { @@ -652,12 +657,7 @@ export class VoiceAssistantWebSocketServer { private readonly ACTIVITY_THRESHOLD_MS = 120_000; - private getClientActivityState(session: Session): { - deviceType: "web" | "mobile" | null; - focusedAgentId: string | null; - isStale: boolean; - appVisible: boolean; - } { + private getClientActivityState(session: Session): ClientAttentionState { const activity = session.getClientActivity(); if (!activity) { return { deviceType: null, focusedAgentId: null, isStale: true, appVisible: false }; @@ -673,58 +673,6 @@ export class VoiceAssistantWebSocketServer { }; } - private computeShouldNotifyForClient( - clientState: { - deviceType: "web" | "mobile" | null; - focusedAgentId: string | null; - isStale: boolean; - appVisible: boolean; - }, - allClientStates: Array<{ - deviceType: "web" | "mobile" | null; - focusedAgentId: string | null; - isStale: boolean; - appVisible: boolean; - }>, - agentId: string - ): boolean { - const isAnyoneActiveOnAgent = allClientStates.some( - (state) => state.focusedAgentId === agentId && state.appVisible && !state.isStale - ); - if (isAnyoneActiveOnAgent) { - return false; - } - - if (clientState.deviceType === null) { - return true; - } - - if (!clientState.isStale && clientState.appVisible && clientState.focusedAgentId !== null) { - return true; - } - - if (!clientState.isStale) { - return false; - } - - const hasActiveWebClient = allClientStates.some( - (state) => state.deviceType === "web" && !state.isStale - ); - - if (clientState.deviceType === "mobile") { - return !hasActiveWebClient; - } - - if (clientState.deviceType === "web") { - const hasOtherClient = allClientStates.some( - (state) => state !== clientState && (state.deviceType === "mobile" || state.deviceType === null) - ); - return !hasOtherClient; - } - - return true; - } - private broadcastAgentAttention(params: { agentId: string; provider: AgentProvider; @@ -732,12 +680,7 @@ export class VoiceAssistantWebSocketServer { }): void { const clientEntries: Array<{ ws: WebSocketLike; - state: { - deviceType: "web" | "mobile" | null; - focusedAgentId: string | null; - isStale: boolean; - appVisible: boolean; - }; + state: ClientAttentionState; }> = []; for (const [ws, connection] of this.sessions) { @@ -749,19 +692,12 @@ export class VoiceAssistantWebSocketServer { const allStates = clientEntries.map((e) => e.state); - const hasActiveWebClient = allStates.some( - (state) => state.deviceType === "web" && !state.isStale - ); - const hasActiveMobileForegroundClient = allStates.some( - (state) => state.deviceType === "mobile" && state.appVisible && !state.isStale - ); - - // Push is only a fallback when the user is away from their desktop/web. + // Push is only a fallback when the user is away from desktop/web. // Also suppress push if they're actively using the mobile app. - const shouldSendPush = - params.reason !== "error" && - !hasActiveWebClient && - !hasActiveMobileForegroundClient; + const shouldSendPush = computeShouldSendPush({ + reason: params.reason, + allClientStates: allStates, + }); if (shouldSendPush) { const tokens = this.pushTokenStore.getAllTokens(); @@ -789,7 +725,11 @@ export class VoiceAssistantWebSocketServer { } for (const { ws, state } of clientEntries) { - const shouldNotify = this.computeShouldNotifyForClient(state, allStates, params.agentId); + const shouldNotify = computeShouldNotifyClient({ + clientState: state, + allClientStates: allStates, + agentId: params.agentId, + }); const message = wrapSessionMessage({ type: "agent_stream",