fix(web-ui): close PR 2428 review gaps

Author: zvzuola

src/web-ui/src/app/scenes/agents/AgentsScene.tsx

@@ -595,15 +595,17 @@ const AgentsHomeView: React.FC = () => {
       await CustomAgentAPI.deleteCustomAgent(id, workspacePath || undefined);
       notification.success(t('agentsOverview.deleteSuccess', { name }));
       closeAgentDetails();
-      await loadAgents();
+      // CustomAgentAPI emits `custom-agent:updated` after the delete; the
+      // useAgentsList subscriber owns the single refresh so two overlapping
+      // catalog loads cannot race their status snapshots.
     } catch (e) {
       notification.error(
         `${t('agentsOverview.deleteFailed')}${e instanceof Error ? e.message : String(e)}`,
       );
     } finally {
       setDeletingAgent(false);
     }
-  }, [selectedAgent, closeAgentDetails, loadAgents, notification, t, workspacePath]);
+  }, [selectedAgent, closeAgentDetails, notification, t, workspacePath]);
 
   const canManageCustomAgent = Boolean(
     selectedAgent

src/web-ui/src/app/scenes/agents/hooks/useAgentsList.ts

@@ -50,6 +50,11 @@ export interface ToolInfo {
   dynamic_info?: DynamicToolInfo;
 }
 
+interface ToolCatalogLoadResult {
+  tools: ToolInfo[];
+  status: ToolCatalogStatus;
+}
+
 interface UseAgentsListOptions {
   searchQuery: string;
   filterLevel: FilterLevel;
@@ -225,25 +230,25 @@ export function useAgentsList({
     // See PR #2428 #3.
     const surfaceTag = renderedPeerDeviceId ?? 'controller';
 
-    const fetchTools = async (): Promise<ToolInfo[]> => {
+    const fetchTools = async (): Promise<ToolCatalogLoadResult> => {
       if (!canQueryToolCatalog) {
         toolLog.info('Tool catalog unsupported on the current peer host; leaving the list empty', { surface: surfaceTag });
-        setToolCatalogStatus('unsupported');
-        return [];
+        return { tools: [], status: 'unsupported' };
       }
       try {
         const tools = await api.invoke<ToolInfo[]>('get_all_tools_info');
-        setToolCatalogStatus(tools.length > 0 ? 'available' : 'empty');
-        return tools;
+        return {
+          tools,
+          status: tools.length > 0 ? 'available' : 'empty',
+        };
       } catch (error) {
         toolLog.error('Failed to load tool catalog', { error });
-        setToolCatalogStatus('failed');
-        return [];
+        return { tools: [], status: 'failed' };
       }
     };
 
     try {
-      const [modes, subagents, tools, configs, reviewTeamDefinition, modelConfigs] = await Promise.all([
+      const [modes, subagents, toolCatalog, configs, reviewTeamDefinition, modelConfigs] = await Promise.all([
         agentAPI.getAvailableModes().catch(() => []),
         SubagentAPI.listSubagents({ workspacePath: workspacePath || undefined }).catch(() => []),
         fetchTools(),
@@ -341,7 +346,8 @@ export function useAgentsList({
       });
 
       setAllAgents([...modeAgents, ...subAgents]);
-      setAvailableTools(tools);
+      setAvailableTools(toolCatalog.tools);
+      setToolCatalogStatus(toolCatalog.status);
       setConfiguredModels(models);
       setModeProfiles(profileMap);
       setAgentSkills(Object.fromEntries(skillEntries));

src/web-ui/src/app/scenes/profile/views/AssistantDefaultsPage.tsx

@@ -1,4 +1,4 @@
-import React, { useCallback, useEffect, useMemo, useState } from 'react';
+import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react';
 import { useTranslation } from 'react-i18next';
 import {
   ArrowLeft,
@@ -106,6 +106,7 @@ const AssistantDefaultsPage: React.FC = () => {
   const [loading, setLoading] = useState(true);
   const [collapsedGroups, setCollapsedGroups] = useState<Set<string>>(new Set());
   const [detail, setDetail] = useState<TemplateDetail | null>(null);
+  const loadRequestIdRef = useRef(0);
 
   // Whether the current host advertises the `tool_catalog` capability. Local
   // always does; a peer host must answer `peer_mode_ping` with tool_catalog.
@@ -128,6 +129,7 @@ const AssistantDefaultsPage: React.FC = () => {
   // writes enabled because there is nothing to toggle anyway. See PR #2428
   // round 5 #2.
   const toolCatalogWritable = toolCatalogStatus === 'available' || toolCatalogStatus === 'empty';
+  const toolCatalogUnavailable = toolCatalogStatus === 'unsupported' || toolCatalogStatus === 'failed';
 
   const skillsEnabled = useMemo(
     () => modeSkills.filter((skill) => skill.effectiveEnabled),
@@ -217,43 +219,54 @@ const AssistantDefaultsPage: React.FC = () => {
   }, [mcpToolsByServer, mcpServers]);
 
   useEffect(() => {
+    const requestId = ++loadRequestIdRef.current;
+
     (async () => {
       setLoading(true);
       try {
         // Skip the tool catalog invoke when the peer host cannot answer it,
         // instead of swallowing the unsupported error as an empty list. The
         // empty list then means "this host doesn't expose a catalog", not
         // "the runtime has no tools".
-        let toolsPromise: Promise<ToolInfo[]>;
+        let toolsPromise: Promise<{
+          tools: ToolInfo[];
+          status: 'available' | 'unsupported' | 'failed' | 'empty';
+        }>;
         if (canQueryToolCatalog) {
           toolsPromise = api.invoke<ToolInfo[]>('get_all_tools_info')
-            .then((tools) => {
-              setToolCatalogStatus(tools.length > 0 ? 'available' : 'empty');
-              return tools;
-            })
+            .then((tools) => ({
+              tools,
+              status: tools.length > 0 ? 'available' as const : 'empty' as const,
+            }))
             .catch((error) => {
               log.error('Failed to load tool catalog', { error });
-              setToolCatalogStatus('failed');
-              return [] as ToolInfo[];
+              return { tools: [] as ToolInfo[], status: 'failed' as const };
             });
         } else {
-          setToolCatalogStatus('unsupported');
-          toolsPromise = Promise.resolve([] as ToolInfo[]);
+          toolsPromise = Promise.resolve({
+            tools: [] as ToolInfo[],
+            status: 'unsupported' as const,
+          });
         }
-        const [modeConf, tools, skillList, servers] = await Promise.all([
+        const [modeConf, toolCatalog, skillList, servers] = await Promise.all([
           configAPI.getAgentProfileConfig(ASSISTANT_MODE_ID).catch(() => null as AgentProfileConfigItem | null),
           toolsPromise,
           configAPI.getModeSkillConfigs({ modeId: ASSISTANT_MODE_ID }).catch(() => [] as ModeSkillInfo[]),
           MCPAPI.getServers().catch(() => [] as MCPServerInfo[]),
         ]);
+        if (requestId !== loadRequestIdRef.current) return;
+
         setAssistantModeConfig(modeConf);
-        setAvailableTools(tools);
+        setAvailableTools(toolCatalog.tools);
+        setToolCatalogStatus(toolCatalog.status);
         setModeSkills(skillList ?? []);
         setMcpServers(servers ?? []);
       } catch (e) {
         log.error('Failed to load assistant defaults config', e);
       } finally {
-        setLoading(false);
+        if (requestId === loadRequestIdRef.current) {
+          setLoading(false);
+        }
       }
     })();
   }, [canQueryToolCatalog, renderedPeerDeviceId]);
@@ -790,22 +803,22 @@ const AssistantDefaultsPage: React.FC = () => {
             <GalleryZone
               title={t('nursery.template.mcpToolsSection')}
             >
-              {mcpServerIds.size === 0 ? (
+              {toolCatalogUnavailable ? (
                 <div className="tc-mcp-empty">
                   <Plug2 size={20} className="tc-mcp-empty__icon" />
-                  {/* The MCP catalog comes from the same get_all_tools_info read
-                      as built-in tools, so an unsupported/failed host affects it
-                      the same way: don't mask it as "no MCP servers". */}
                   <span className="tc-mcp-empty__text">
                     {toolCatalogStatus === 'unsupported'
                       ? t('empty.toolsUnsupported')
-                      : toolCatalogStatus === 'failed'
-                        ? t('empty.toolsFailed')
-                        : t('nursery.template.mcpEmptyTitle')}
+                      : t('empty.toolsFailed')}
+                  </span>
+                </div>
+              ) : mcpServerIds.size === 0 ? (
+                <div className="tc-mcp-empty">
+                  <Plug2 size={20} className="tc-mcp-empty__icon" />
+                  <span className="tc-mcp-empty__text">
+                    {t('nursery.template.mcpEmptyTitle')}
                   </span>
-                  {toolCatalogStatus !== 'unsupported' && toolCatalogStatus !== 'failed' ? (
-                    <span className="tc-mcp-empty__hint">{t('nursery.template.mcpEmptyHint')}</span>
-                  ) : null}
+                  <span className="tc-mcp-empty__hint">{t('nursery.template.mcpEmptyHint')}</span>
                 </div>
               ) : (
                 <div className="tc-tool-groups">

src/web-ui/src/infrastructure/peer-device/PeerConnectionManager.test.ts

@@ -33,18 +33,41 @@ describe('PeerConnectionManager attach', () => {
     expect(manager.get('peer-1')).toBe(connection);
   });
 
-  it('reports new capability fields as null (unknown) when the host omits them', async () => {
-    // An older host's peer_mode_ping does not advertise cancel_tool /
-    // tool_catalog; coercing those to `false` would hide a working capability
-    // on an older Desktop (Interrupt button / tool list). They must parse to
-    // `null` so consumers stay optimistic. See PR #2428 #4.
-    const rpc = createRpc();
+  it('classifies a truly old Desktop from its supported tool catalog probe', async () => {
+    const rpc = createLegacyRpc('desktop');
     const manager = createManager(rpc.deviceRpc);
 
     const connection = await manager.connect('peer-1', 'Studio');
-    const caps = connection.getState().capabilities;
-    expect(caps.cancelTool).toBeNull();
-    expect(caps.toolCatalog).toBeNull();
+    expect(connection.getState().capabilities).toMatchObject({
+      cancelTool: true,
+      toolCatalog: true,
+      hostKind: 'desktop',
+    });
+    expect(rpc.commands()).toEqual([
+      'peer_mode_ping',
+      'get_all_tools_info',
+      'peer_control_attach',
+    ]);
+
+    await vi.advanceTimersByTimeAsync(KEEPALIVE_MS);
+    expect(rpc.commands().filter(command => command === 'get_all_tools_info')).toHaveLength(1);
+  });
+
+  it('classifies a truly old CLI from an unsupported tool catalog probe', async () => {
+    const rpc = createLegacyRpc('cli');
+    const manager = createManager(rpc.deviceRpc);
+
+    const connection = await manager.connect('peer-1', 'CLI');
+    expect(connection.getState().capabilities).toMatchObject({
+      cancelTool: false,
+      toolCatalog: false,
+      hostKind: 'cli',
+    });
+    expect(rpc.commands()).toEqual([
+      'peer_mode_ping',
+      'get_all_tools_info',
+      'peer_control_attach',
+    ]);
   });
 
   it('parses advertised new capability fields as true', async () => {
@@ -417,6 +440,70 @@ describe('PeerConnectionManager disposal', () => {
     // A snapshot was published for the capability change while staying ready.
     expect(snapshots.length).toBeGreaterThan(0);
   });
+
+  it('does not let a stale health probe classify a replacement connection', async () => {
+    const healthCatalog = deferred<string>();
+    const commands: string[] = [];
+    let catalogCallCount = 0;
+    const deviceRpc = vi.fn(async (_target: string, commandJson: string): Promise<string> => {
+      const command = (JSON.parse(commandJson) as { command?: string }).command ?? 'unknown';
+      commands.push(command);
+
+      if (command === 'peer_mode_ping') {
+        return JSON.stringify({
+          resp: 'host_invoke_result',
+          ok: true,
+          value: { capabilities: {} },
+        });
+      }
+      if (command === 'get_all_tools_info') {
+        catalogCallCount += 1;
+        if (catalogCallCount === 1) {
+          throw new Error('relay unavailable');
+        }
+        if (catalogCallCount === 2) {
+          return healthCatalog.promise;
+        }
+        throw new Error('relay unavailable');
+      }
+      return JSON.stringify({ resp: 'host_invoke_result', ok: true, value: null });
+    });
+    const manager = createManager(deviceRpc);
+
+    const first = await manager.connect('peer-1', 'Studio');
+    expect(first.getState().capabilities).toMatchObject({
+      cancelTool: null,
+      toolCatalog: null,
+      hostKind: null,
+    });
+
+    await vi.advanceTimersByTimeAsync(KEEPALIVE_MS);
+    await vi.waitFor(() => expect(commands.filter(command => command === 'get_all_tools_info')).toHaveLength(2));
+
+    await manager.dispose('peer-1', { notifyPeer: false });
+    const replacement = await manager.connect('peer-1', 'Studio');
+    expect(replacement.getState().capabilities).toMatchObject({
+      cancelTool: null,
+      toolCatalog: null,
+      hostKind: null,
+    });
+
+    expect(catalogCallCount).toBe(3);
+    healthCatalog.resolve(JSON.stringify({
+      resp: 'host_invoke_result',
+      ok: true,
+      value: [],
+    }));
+    await Promise.resolve();
+
+    await vi.advanceTimersByTimeAsync(KEEPALIVE_MS);
+    expect(catalogCallCount).toBe(4);
+    expect(replacement.getState().capabilities).toMatchObject({
+      cancelTool: null,
+      toolCatalog: null,
+      hostKind: null,
+    });
+  });
 });
 
 function createManager(
@@ -469,9 +556,12 @@ function createRpc(options: { failCommands?: Set<string> } = {}) {
         resp: 'host_invoke_result',
         ok: true,
         value: {
+          host_type: 'desktop',
           capabilities: {
             idempotent_dialog_submit: true,
             token_usage_statistics: true,
+            cancel_tool: true,
+            tool_catalog: true,
           },
         },
       });
@@ -495,6 +585,52 @@ function createRpc(options: { failCommands?: Set<string> } = {}) {
   };
 }
 
+function createLegacyRpc(kind: 'desktop' | 'cli') {
+  const calls: RpcCall[] = [];
+  const deviceRpc = vi.fn(async (_target: string, commandJson: string): Promise<string> => {
+    const parsed = JSON.parse(commandJson) as { command?: string; args?: Record<string, unknown> };
+    const command = parsed.command ?? 'unknown';
+    calls.push({ command, args: parsed.args ?? {} });
+
+    if (command === 'peer_mode_ping') {
+      return JSON.stringify({
+        resp: 'host_invoke_result',
+        ok: true,
+        value: {
+          ok: true,
+          peer: true,
+          device_id: 'legacy-peer',
+          capabilities: {
+            idempotent_dialog_submit: true,
+            targeted_session_rollback: true,
+            token_usage_statistics: true,
+          },
+        },
+      });
+    }
+    if (command === 'get_all_tools_info') {
+      if (kind === 'cli') {
+        return JSON.stringify({
+          resp: 'host_invoke_result',
+          ok: false,
+          error: "command 'get_all_tools_info' is not supported on CLI peer host",
+        });
+      }
+      return JSON.stringify({
+        resp: 'host_invoke_result',
+        ok: true,
+        value: [],
+      });
+    }
+    return JSON.stringify({ resp: 'host_invoke_result', ok: true, value: null });
+  });
+
+  return {
+    deviceRpc,
+    commands: () => calls.map(call => call.command),
+  };
+}
+
 function deferred<T>() {
   let resolve!: (value: T) => void;
   const promise = new Promise<T>(res => {