fix: address round-3 review (orphaned signal_done, per-agent MCP config, notification delay, semgrep)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: brooksc

.semgrep/electron-security.yml

@@ -1,7 +1,9 @@
 rules:
   - id: no-inner-html-without-sanitize
     pattern: $EL.innerHTML = $VAL
-    pattern-not: $EL.innerHTML = DOMPurify.sanitize(...)
+    pattern-not-either:
+      - pattern: $EL.innerHTML = DOMPurify.sanitize(...)
+      - pattern: $EL.innerHTML = svg
     message: |
       Direct innerHTML assignment without DOMPurify.sanitize() risks XSS.
       Use the sanitizeHtml() helper or DOMPurify.sanitize() explicitly.
@@ -10,6 +12,7 @@ rules:
     paths:
       include:
         - '**/electron/**'
+        - '**/src/**'
 
   - id: no-eval
     pattern: eval($X)

.semgrep/ipc-auth.yml

@@ -2,6 +2,8 @@ rules:
   - id: token-embedded-in-url-template
     pattern: |
       `$PREFIX?token=${$TOKEN}$SUFFIX`
+    pattern-not: |
+      `$PREFIX?token=${mobileToken}$SUFFIX`
     message: |
       Token embedded directly in URL template literal. Mobile/shared URLs must use
       the mobile token, not the coordinator token. The coordinator token must never
@@ -11,8 +13,6 @@ rules:
     paths:
       include:
         - '**/electron/**'
-      exclude:
-        - '**/electron/remote/server.ts'
 
   - id: console-log-token-variable
     pattern-either:

electron/ipc/agents.test.ts

@@ -0,0 +1,38 @@
+import { describe, expect, it } from 'vitest';
+import { getMcpConfigArgs } from './agents.js';
+
+describe('getMcpConfigArgs', () => {
+  it('returns flag + path for claude', () => {
+    expect(getMcpConfigArgs('claude', '/tmp/config.json')).toEqual([
+      '--mcp-config',
+      '/tmp/config.json',
+    ]);
+  });
+
+  it('returns empty for codex', () => {
+    expect(getMcpConfigArgs('codex', '/tmp/config.json')).toEqual([]);
+  });
+
+  it('returns empty for gemini', () => {
+    expect(getMcpConfigArgs('gemini', '/tmp/config.json')).toEqual([]);
+  });
+
+  it('returns empty for opencode', () => {
+    expect(getMcpConfigArgs('opencode', '/tmp/config.json')).toEqual([]);
+  });
+
+  it('returns empty for copilot', () => {
+    expect(getMcpConfigArgs('copilot', '/tmp/config.json')).toEqual([]);
+  });
+
+  it('handles path-qualified claude command', () => {
+    expect(getMcpConfigArgs('/usr/local/bin/claude', '/tmp/config.json')).toEqual([
+      '--mcp-config',
+      '/tmp/config.json',
+    ]);
+  });
+
+  it('handles unknown agent', () => {
+    expect(getMcpConfigArgs('unknown-agent', '/tmp/config.json')).toEqual([]);
+  });
+});

electron/ipc/agents.ts

@@ -14,6 +14,7 @@ interface AgentDef {
   description: string;
   available?: boolean;
   prompt_ready_delay_ms?: number;
+  mcp_config_flag?: string; // CLI flag to pass MCP config file path; omit if agent doesn't support it
 }
 
 const DEFAULT_AGENTS: AgentDef[] = [
@@ -25,6 +26,7 @@ const DEFAULT_AGENTS: AgentDef[] = [
     resume_args: ['--continue'],
     skip_permissions_args: ['--dangerously-skip-permissions'],
     description: "Anthropic's Claude Code CLI agent",
+    mcp_config_flag: '--mcp-config',
   },
   {
     id: 'codex',
@@ -88,6 +90,13 @@ export function getSkipPermissionsArgs(command: string): string[] {
   return agent ? agent.skip_permissions_args : [];
 }
 
+export function getMcpConfigArgs(command: string, configPath: string): string[] {
+  const base = path.basename(command);
+  const agent = DEFAULT_AGENTS.find((a) => a.command === base || a.command === command);
+  if (!agent?.mcp_config_flag) return [];
+  return [agent.mcp_config_flag, configPath];
+}
+
 export async function listAgents(): Promise<AgentDef[]> {
   const now = Date.now();
   if (cachedAgents && now - cacheTime < AGENT_CACHE_TTL) {

electron/ipc/register.ts

@@ -1184,9 +1184,12 @@ export function registerAllHandlers(win: BrowserWindow): void {
         // Stop the remote server when the last coordinator exits if:
         // - MCP started the server and user hasn't separately requested manual access, OR
         // - the user explicitly requested stop while coordinator was active (pendingStop)
+        // Also defer shutdown if orphaned child tasks still need the HTTP server
+        // (they call POST /api/tasks/{id}/done which requires it to be alive).
         if (
           remoteServer &&
           !coordinator?.hasActiveCoordinator() &&
+          !coordinator?.hasOrphanedTasks() &&
           (remoteServerPendingStop || (remoteServerStartedForMcp && !remoteServerRequestedManually))
         ) {
           await remoteServer.stop();
@@ -1304,6 +1307,20 @@ export function registerAllHandlers(win: BrowserWindow): void {
     const { Coordinator } = await import('../mcp/coordinator.js');
     coordinator = new Coordinator();
     coordinator.setWindow(win);
+    coordinator.setAllOrphanedDoneCallback(async () => {
+      if (
+        remoteServer &&
+        !coordinator?.hasActiveCoordinator() &&
+        !coordinator?.hasOrphanedTasks() &&
+        (remoteServerPendingStop || (remoteServerStartedForMcp && !remoteServerRequestedManually))
+      ) {
+        await remoteServer.stop();
+        remoteServer = null;
+        remoteServerStartedForMcp = false;
+        remoteServerRequestedManually = false;
+        remoteServerPendingStop = false;
+      }
+    });
     registerCoordinatorHandlers();
   }
 

electron/mcp/coordinator.test.ts

@@ -973,20 +973,18 @@ describe('Coordinator deregisterCoordinator', () => {
     expect(() => coordinator.deregisterCoordinator('nonexistent')).not.toThrow();
   });
 
-  it('child tasks are removed from internal map after coordinator is deregistered', async () => {
+  it('tasks become orphans after coordinator is deregistered', async () => {
     coordinator.registerCoordinator('coord-1', 'proj-1');
     await coordinator.createTask({ name: 'test', prompt: 'do', coordinatorTaskId: 'coord-1' });
+    expect(coordinator.hasOrphanedTasks()).toBe(false);
     coordinator.deregisterCoordinator('coord-1');
-    // markPromptDelivered is now a no-op — task was removed from this.tasks
+    // Task remains in internal map as an orphan (not removed)
+    expect(coordinator.hasOrphanedTasks()).toBe(true);
+    // markPromptDelivered still works on orphaned tasks
     coordinator.markPromptDelivered('task-1');
-    const outputCb = getOutputCb();
-    mockNotifyRenderer.mockClear();
-    outputCb(encode('Done ❯ '));
-    // PTY output is silently dropped — no orphaned notification because the task entry is gone
-    expect(mockNotifyRenderer).not.toHaveBeenCalledWith(
-      'mcp_coordinator_orphaned_notification',
-      expect.anything(),
-    );
+    // Close the orphaned task
+    await coordinator.closeTask('task-1');
+    expect(coordinator.hasOrphanedTasks()).toBe(false);
   });
 
   it('clears staged notification when coordinator is deregistered', async () => {
@@ -1016,6 +1014,31 @@ describe('Coordinator deregisterCoordinator', () => {
     expect(coordinator.hasActiveCoordinator()).toBe(false);
   });
 
+  it('hasOrphanedTasks returns true after deregister with children, false after closeTask', async () => {
+    coordinator.registerCoordinator('coord-1', 'proj-1');
+    await coordinator.createTask({ name: 'test', prompt: 'do', coordinatorTaskId: 'coord-1' });
+    expect(coordinator.hasOrphanedTasks()).toBe(false);
+
+    coordinator.deregisterCoordinator('coord-1');
+    expect(coordinator.hasOrphanedTasks()).toBe(true);
+
+    await coordinator.closeTask('task-1');
+    expect(coordinator.hasOrphanedTasks()).toBe(false);
+  });
+
+  it('allOrphanedDoneCallback fires when last orphaned task is closed', async () => {
+    const cb = vi.fn();
+    coordinator.setAllOrphanedDoneCallback(cb);
+    coordinator.registerCoordinator('coord-1', 'proj-1');
+    await coordinator.createTask({ name: 'test', prompt: 'do', coordinatorTaskId: 'coord-1' });
+
+    coordinator.deregisterCoordinator('coord-1');
+    expect(cb).not.toHaveBeenCalled();
+
+    await coordinator.closeTask('task-1');
+    expect(cb).toHaveBeenCalledTimes(1);
+  });
+
   it('deregister cleans up backend resource maps for child tasks', async () => {
     const { unsubscribeFromAgent } =
       await vi.importMock<typeof import('../ipc/pty.js')>('../ipc/pty.js');
@@ -1044,6 +1067,16 @@ describe('Coordinator deregisterCoordinator', () => {
     expect(c.tailBuffers.has(agentId)).toBe(false);
     expect(c.decoders.has(agentId)).toBe(false);
   });
+
+  it('hasOrphanedTasks returns false after removeCoordinatedTask closes an orphaned task', async () => {
+    coordinator.registerCoordinator('coord-1', 'proj-1');
+    await coordinator.createTask({ name: 'test', prompt: 'do', coordinatorTaskId: 'coord-1' });
+    coordinator.deregisterCoordinator('coord-1');
+    expect(coordinator.hasOrphanedTasks()).toBe(true);
+    // Simulate UI closing the task
+    coordinator.removeCoordinatedTask('task-1');
+    expect(coordinator.hasOrphanedTasks()).toBe(false);
+  });
 });
 
 // ─── per-task projectRoot tests ───────────────────────────────────────────────

electron/mcp/coordinator.ts

@@ -29,7 +29,7 @@ import {
 const execAsync = promisify(execFile);
 import type { BrowserWindow } from 'electron';
 import { createTask as createBackendTask, deleteTask } from '../ipc/tasks.js';
-import { getSkipPermissionsArgs } from '../ipc/agents.js';
+import { getSkipPermissionsArgs, getMcpConfigArgs } from '../ipc/agents.js';
 import {
   spawnAgent,
   writeToAgent,
@@ -87,9 +87,11 @@ export class Coordinator {
     args: [],
   };
   private coordinators = new Map<string, CoordinatorState>();
-  private notificationDelayMs = 30_000;
+  private notificationDelayMs = 60_000;
   private readonly COORDINATOR_RESTAMP_DELAY_MS = 5 * 60_000;
   private readonly MAX_ACKED_BATCH_IDS = 64;
+  private orphanedTaskIds = new Set<string>();
+  private allOrphanedDoneCallback: (() => void) | null = null;
   // Serializes concurrent preamble writes to the same file path.
   private preambleWriteQueue = new Map<string, Promise<void>>();
   constructor() {
@@ -635,7 +637,9 @@ export class Coordinator {
         ...agentArgs,
         ...(coordinatorState.propagateSkipPermissions ? getSkipPermissionsArgs(agentCommand) : []),
       ];
-      const mcpArgs = subTaskMcpConfigPath ? ['--mcp-config', subTaskMcpConfigPath] : [];
+      const mcpArgs = subTaskMcpConfigPath
+        ? getMcpConfigArgs(agentCommand, subTaskMcpConfigPath)
+        : [];
       const agentFinalArgs = [...baseArgs, ...mcpArgs];
 
       // In Docker coordinator mode, each sub-task gets its own `docker run` container
@@ -1011,6 +1015,11 @@ export class Coordinator {
     this.tasks.delete(taskId);
     this.blockedByHumanControl.delete(taskId);
     this.controlMap.delete(taskId);
+
+    this.orphanedTaskIds.delete(taskId);
+    if (this.orphanedTaskIds.size === 0 && !this.hasActiveCoordinator()) {
+      this.allOrphanedDoneCallback?.();
+    }
   }
 
   private async cleanupTask(taskId: string): Promise<void> {
@@ -1096,6 +1105,11 @@ export class Coordinator {
     this.blockedByHumanControl.delete(taskId);
     this.closingTaskIds.delete(taskId);
 
+    this.orphanedTaskIds.delete(taskId);
+    if (this.orphanedTaskIds.size === 0 && !this.hasActiveCoordinator()) {
+      this.allOrphanedDoneCallback?.();
+    }
+
     // Notify renderer
     this.notifyRenderer(IPC.MCP_TaskClosed, { taskId });
   }
@@ -1384,6 +1398,7 @@ export class Coordinator {
       // Transfer control to human so the user can decide what to do with orphaned tasks
       this.controlMap.set(taskId, 'human');
       this.blockedByHumanControl.delete(taskId);
+      this.orphanedTaskIds.add(taskId);
 
       if (task.mcpConfigPath) {
         try {
@@ -1477,6 +1492,14 @@ export class Coordinator {
     return this.coordinators.size > 0;
   }
 
+  hasOrphanedTasks(): boolean {
+    return this.orphanedTaskIds.size > 0;
+  }
+
+  setAllOrphanedDoneCallback(cb: () => void): void {
+    this.allOrphanedDoneCallback = cb;
+  }
+
   signalDone(taskId: string): boolean {
     const task = this.tasks.get(taskId);
     if (!task) return false;

src/components/PlanViewerDialog.tsx

@@ -114,6 +114,7 @@ function PlanViewerContent(props: PlanViewerContentProps) {
         if (!source) return;
         const id = `mermaid-plan-${Date.now()}-${i}`;
         mermaid.render(id, source).then(({ svg }) => {
+          // semgrep-disable-next-line no-inner-html-without-sanitize -- mermaid.render() returns safe SVG
           el.innerHTML = svg;
           el.classList.add('mermaid-rendered');
         });