feat(coordinator): MCP orchestration engine, REST API hardening, and task model (PR 2/4)

[brooksc]

Overview

This is PR 2 of 4 in the coordinator series splitting #100 as requested in the round-4 review. It is stacked on PR 1 (coordinator-1-security) and should be merged after that one. The diff shown here includes PR 1's content; the meaningful delta for this PR is the coordinator engine and REST hardening described below.

PR sequence:

PR	Branch	Status	Contents
1	coordinator-1-security	Open	Atomic writes, input validators, static analysis configs
2 (this PR)	coordinator-2-mcp-backend	Open	MCP coordinator engine + REST API hardening
3	coordinator-3-store-ipc	Pending	Frontend store wiring + IPC handlers
4	coordinator-4-ui	Pending	UI components + coordinator entry points

PRs 3–4 are stacked on this one. Nothing coordinator-related is user-visible until PR 4 adds the NewTaskDialog checkbox and Settings toggle (coordinatorModeEnabled defaults to false).

---

What's in this PR (delta over PR 1)

MCP orchestration engine (electron/mcp/)

coordinator.ts — core orchestrator singleton

• Creates and manages coordinated sub-tasks, each in its own git worktree
• Three-class token RBAC: coordinator / subtask / mobile
• Per-task done tokens (24-byte random, timing-safe comparison)
• PTY output subscription for idle/prompt detection
• Batched review notifications with configurable delay and restaging
• signal_done / waitForSignalDone with replay cache (requestId dedup for safe retries)
• Atomic preamble injection and strip via atomic.ts
• setTaskControl / blockedByHumanControl state machine (coordinator ↔ human hand-off)
• cleanupTask with double-resolve guard on anySignalResolvers

server.ts — MCP stdio entry point

• Speaks MCP over stdio to Claude Code; delegates to Electron app via HTTP
• CLI arg validation: rejects \r/\n in --coordinator-id / --task-id (header injection prevention)
• Exposes tools: create_task, list_tasks, get_task_status, get_task_diff, get_task_output, send_prompt, wait_for_idle, wait_for_signal_done, merge_task, close_task, review_and_merge_task, signal_done

config.ts — MCP JSON config generation

• selectMcpJsonDir: selects the right directory for the per-coordinator config
• writeMcpConfig: writes config (mode 0o600) atomically

preamble.ts — preamble injection and strip

• Atomically appends a <sub-task-mode> block to CLAUDE.md, AGENTS.md, GEMINI.md, .agent.md, or settings.local.json depending on agent type
• stripPreambleFromBranch: atomically strips the block on merge/close
• buildNormalizedPreambleFileDiff: git diff --no-index with anchored-regex path rewriting (prevents false substitutions when tmpdir path appears in file content)

sub-task-preamble.ts — sub-task-side preamble injection
prompt-detect.ts — sliding-window idle/prompt detector
replay-cache.ts — deduplicates wait_for_signal_done retries by requestId
client.ts — MCPClient used by the MCP stdio process to call the REST API
mcp-tool-list.ts — builds the tool list advertised to coordinator vs. sub-task roles
types.ts — shared types: CoordinatedTask, ApiTaskSummary, ApiTaskDetail, token classes, etc.

---

REST API hardening (electron/remote/server.ts)

New coordinator task routes (all require auth):

Method	Path	Description
POST	/api/tasks	Create sub-task
GET	/api/tasks	List sub-tasks (coordinator-scoped)
GET	/api/tasks/:id	Get status
POST	/api/tasks/:id/prompt	Send prompt
POST	/api/tasks/:id/wait	Wait for idle
GET	/api/tasks/:id/diff	Get diff
GET	/api/tasks/:id/output	Get scrollback
POST	/api/tasks/:id/merge	Merge branch
POST	/api/tasks/:id/review-merge	Diff + merge
DELETE	/api/tasks/:id	Close/cleanup
POST	/api/tasks/:id/done	Signal done (subtask token + X-Done-Token)
POST	/api/wait-signal	Wait for any signal_done

Auth scoping hardening:

• callerCoordinatorId extracted from verified X-Coordinator-Id header and enforced before all coordinator routes (including wait-signal)
• create_task: body coordinatorTaskId ignored; header is authoritative; mismatch → 403
• wait-signal: body coordinatorTaskId ignored entirely; header-only
• Mobile token restricted to /api/agents only — task routes removed (mobile token is embedded in a QR-code URL reachable by anyone on the local network)
• Coordinator token without X-Coordinator-Id → 403 on all task routes
• task.name: 200-char max, control characters stripped (prompt injection prevention)

---

IPC handlers (electron/ipc/register.ts)

New handlers wired up: StartMCPServer, StopMCPServer, GetMCPStatus, GetMCPLogs, HydrateCoordinatedTask, MCP_CoordinatedTaskClosed, MCP_TaskHydrated, MCP_CoordinatorNotificationAck, MCP_CoordinatorNotificationDropAck, MCP_CoordinatorRestageAfterUserSend

---

Store / type changes (src/store/)

New Task fields: coordinatorMode, coordinatedBy, controlledBy, mcpConfigPath, preambleFileExistedBefore, signalDone*, needsReview, mcpStartupStatus/Error

New PersistedState / AppStore fields: coordinatorModeEnabled, coordinatorNotificationDelayMs, coordinatorControlHintDismissed, MCPStatus

core.ts, remote.ts, persistence.ts, store.ts, ui.ts — wired up coordinator store state with defaults and persistence round-trip

---

OpenSpec

openspec/changes/coordinator-mcp-backend/proposal.md — documents the MCP orchestration server, REST task API, three-token auth model, signal/wait lifecycle, preamble injection, and new IPC channels per CLAUDE.md requirement

---

Tests

File	Cases	Coverage
electron/mcp/coordinator.test.ts	180+	Lifecycle, preamble injection, waitForIdle, signal/wait, notifications, hydrateTask, setTaskControl, cleanupTask
electron/mcp/coordinator-sequence.test.ts	10+	End-to-end create → signal → merge
electron/mcp/config.test.ts	15	selectMcpJsonDir, writeMcpConfig
electron/mcp/mcp-tool-list.test.ts	10	Tool selection by role
electron/mcp/prompt-detect.test.ts	15	Idle detection patterns
electron/remote/coordinator-scoping.test.ts	40	HTTP integration: coordinator scoping, subtask token restrictions, mobile token restrictions, create_task body-vs-header scoping
electron/ipc/register-mcp.test.ts	10	StartMCPServer input validation
electron/ipc/register.test.ts	5	IPC handler registration
electron/ipc/docker-config.test.ts	10	Docker MCP config paths
electron/mcp/docker.integration.test.ts	—	Docker lifecycle (skipped without Docker daemon)

Total: 295+ test cases. npm test → 859 pass, 12 skipped.

Test plan

• npm run compile && npm run typecheck && npm run lint && npm run format:check — pass
• npm test — 859 pass, 12 skipped (Docker integration skipped without daemon)
• git diff --check johannesjo/main...HEAD — clean

🤖 Generated with Claude Code

[johannesjo]

Thank you very much for your work on this! <3

Follow-up review after splitting the pass across remote auth, coordinator lifecycle/persistence, and MCP execution paths. I think these need attention before merge:

1. Connect Phone cannot authenticate with the new mobile token. The QR URL now embeds mobileToken (electron/remote/server.ts:867), but WebSocket auth still accepts only coordinator tokens (electron/remote/server.ts:760). The phone SPA sends the QR token as its first WS auth message (src/remote/ws.ts:41), so scanning the QR gets 4001 Unauthorized and reloads as unauthenticated. Either allow mobile-scoped WS access to the existing agent protocol, or move the mobile UI to a REST-only read-only flow.

2. Connect Phone can return unreachable LAN URLs while MCP is active. If MCP started the shared remote server on 127.0.0.1, StartRemoteServer intentionally skips rebinding while a coordinator is active (electron/ipc/register.ts:1061) but still returns wifiUrl/tailscaleUrl from that same loopback-bound server (electron/remote/server.ts:878). The modal will show QR URLs that other devices cannot reach. Track the bind host and either return an explicit unavailable state, run a separate LAN/mobile server, or only expose LAN URLs after a real 0.0.0.0 bind.

3. Coordinator state is typed but not persisted/restored. PersistedState/PersistedTask gained coordinator fields, and main-process startup checks coordinatorModeEnabled (electron/ipc/register.ts:1306), but saveState() does not write those top-level fields (src/store/persistence.ts:86) and task restore omits coordinatedBy, controlledBy, mcpConfigPath, signalDone*, needsReview, etc. (src/store/persistence.ts:558, src/store/persistence.ts:641). Restart loses the coordinator setting, child nesting, MCP config paths, signal state, and review state, so hydration cannot work reliably.

4. Deregistering a coordinator drops live child-task backend state. deregisterCoordinator() unsubscribes each child PTY and deletes the task from this.tasks (electron/mcp/coordinator.ts:1361, electron/mcp/coordinator.ts:1391) without necessarily killing those child agents/worktrees. A later signal_done for that still-running child will be task-not-found, and real PTY output will no longer drive orphan/review notifications. Keep child task records until each child is closed, or explicitly transfer them to human/orphaned review state while preserving the done endpoint.

5. Sub-task agent args are Claude-specific even when the coordinator agent is not Claude. createTask() appends --mcp-config and --dangerously-skip-permissions to every configured agent command (electron/mcp/coordinator.ts:633, electron/mcp/coordinator.ts:637). The rest of the app models skip-permission args per agent (src/components/TaskAITerminal.tsx:592), so Codex/Gemini/opencode sub-tasks will be misconfigured or fail to start. Either restrict coordinator sub-tasks to supported agents, or make MCP config and skip-permission injection agent-specific.

Secondary follow-ups from the pass: ConnectPhoneModal ignores { stopped: false, reason: 'coordinator_active' } from stopRemoteAccess() (src/components/ConnectPhoneModal.tsx:145), and StartMCPServer should validate skipPermissions/propagateSkipPermissions as booleans plus UUID-check coordinatorTaskId before using it in temp paths.

[brooksc]

Thank you for the thorough review — all five issues and both secondary items are addressed in the latest push. Additional correctness issues surfaced during internal review and are also fixed; listed below.

---

1. Connect Phone cannot authenticate with the new mobile token

The WS auth handler in electron/remote/server.ts now accepts both 'coordinator' and 'mobile' token types via classifyCandidate(). A Map<WebSocket, 'coordinator' | 'mobile'> tracks the token type per connection. A read-only guard before the message dispatch closes the socket with 4003 Forbidden if a mobile client sends input, resize, or kill — so phone clients get live terminal output with no write access.

2. Connect Phone can return unreachable LAN URLs while MCP is active

bindHost: string is now tracked on the RemoteServer object (set from opts.host at bind time). In StartRemoteServer, when the existing server is loopback-bound (bindHost === '127.0.0.1'), we return { wifiUrl: null, tailscaleUrl: null, unavailableReason: 'coordinator_active' } without mutating remoteServerRequestedManually or remoteServerPendingStop — preserving the coordinator's cleanup path. GetRemoteStatus also returns { enabled: false } when the server is loopback-only, so the Connect Phone modal cannot re-activate via status polling. On the frontend, startRemoteAccess() throws a descriptive error when unavailableReason is present. stopRemoteAccess() returns { stopped: boolean; reason?: string } and ConnectPhoneModal.handleDisconnect() checks the return value, showing an informational message rather than silently closing when the coordinator is blocking the stop.

3. Coordinator state is typed but not persisted/restored

saveState() in src/store/persistence.ts now writes all coordinator fields:

• 3 global fields: coordinatorModeEnabled, coordinatorNotificationDelayMs, coordinatorControlHintDismissed
• 10 per-task fields in both active and collapsed task sections: coordinatorMode, propagateSkipPermissions, coordinatedBy, controlledBy, mcpConfigPath, preambleFileExistedBefore, signalDoneReceived, signalDoneAt, signalDoneConsumed, needsReview

loadState() restores all of the above with appropriate type guards.

4. Deregistering a coordinator drops live child-task backend state

deregisterCoordinator() in electron/mcp/coordinator.ts no longer calls this.tasks.delete(taskId) on children. Instead each child is marked orphaned: task.needsReview = true, controlMap set to 'human', and MCP_TaskStateSync fired to the renderer. The PTY output subscription is removed but the task record is preserved, so subsequent signal_done calls for still-running children resolve correctly.

5. Sub-task agent args are Claude-specific even when the coordinator agent is not Claude

electron/ipc/agents.ts exports getSkipPermissionsArgs(command: string): string[], which normalises via path.basename() (handling path-qualified executables) and returns the correct per-agent flag from AgentDef.skip_permissions_args. createTask() in coordinator.ts spreads the result instead of hardcoding --dangerously-skip-permissions. Codex gets --dangerously-bypass-approvals-and-sandbox; Gemini/Copilot get --yolo; opencode gets nothing.

---

Secondary: ConnectPhoneModal ignores { stopped: false } from stopRemoteAccess()

Addressed in item 2 above — handleDisconnect() now checks the return value and surfaces the coordinator-active reason.

Secondary: StartMCPServer validation

validateStartMCPServerArgs() now uses the existing validateUUID() helper (strict UUID v4 regex) for coordinatorTaskId, and adds assertOptionalBoolean checks for skipPermissions and propagateSkipPermissions. Test fixtures in register-mcp.test.ts were migrated from short stub IDs to a valid UUID constant to match the stricter validation.

---

Additional fixes from internal review

• Compile blocker: duplicate token classifier — An earlier draft introduced a second classifyToken(candidate: string) overload that called an undefined safeCompare. Removed; the auth handler uses classifyCandidate() directly throughout.

• GetRemoteStatus re-enabling dead QR — Before this fix, GetRemoteStatus returned enabled: true even when the server was loopback-only, allowing the Connect Phone modal to display a reachable-looking status after a coordinator-active rejection. Fixed by returning enabled: false when bindHost === '127.0.0.1'.

• Failed manual start requests poisoning MCP cleanup — remoteServerRequestedManually and remoteServerPendingStop were being set before the loopback check, so a coordinator-active rejection would leave the flags in the wrong state and interfere with the MCP server's own teardown. Fixed by moving those assignments to after the check.

• Write UI visible on read-only mobile sessions — src/remote/AgentDetail.tsx previously rendered an input field and quick-action buttons even though mobile is always read-only. Removed all write UI; the toolbar now shows only A−/A+ font-size controls.

• Path-qualified commands getting no skip args — getSkipPermissionsArgs('/usr/local/bin/claude') returned [] because the exact-match lookup failed on full paths. Fixed with path.basename() normalisation before the lookup.

[johannesjo]

Thank you very much for your ongoing work on this! <3

Additional review of 2465ab6..b43a427 after splitting the pass across coordinator lifecycle, remote/mobile auth, and tooling/agent compatibility.

The mobile WebSocket/auth and loopback remote-status changes look good from the additional pass. I still think these need attention before merge:

1. Orphaned sub-tasks still lose the signal_done path. deregisterCoordinator() now keeps child task records (electron/mcp/coordinator.ts:1362), but the IPC deregistration handler stops the shared MCP HTTP server as soon as the last coordinator exits when it was MCP-started and not manually requested (electron/ipc/register.ts:1187, electron/ipc/register.ts:1192). In the normal host path that leaves still-running sub-tasks with a dead 127.0.0.1:<port> endpoint, so a later signal_done fails before Coordinator.signalDone() can use the retained task record. The same orphaning path also unlinks and clears each child mcpConfigPath (electron/mcp/coordinator.ts:1388, electron/mcp/coordinator.ts:1394), so delayed/lazy MCP startup can lose its config too. Either keep the MCP transport/config alive until all children are closed, or explicitly terminate/convert the children so the system no longer promises that later signal_done calls will resolve.

2. Sub-task agent args are still partly Claude-specific. The skip-permission flag is now selected via getSkipPermissionsArgs() (electron/mcp/coordinator.ts:636, electron/ipc/agents.ts:85), but createTask() still appends --mcp-config <path> to every selected agent (electron/mcp/coordinator.ts:638). Non-Claude or custom agents that do not support that Claude-style flag can still fail or start with an unknown argument. This should be gated to supported agents or made per-agent along with skip-permission propagation.

3. The coordinator notification delay default does not round-trip to the backend. The renderer default is 60_000 (src/store/core.ts:88) and saveState() omits that value when it equals the default (src/store/persistence.ts:129), but the main-process coordinator default remains 30_000 (electron/mcp/coordinator.ts:90) and syncTaskNamesFromJson() only calls setNotificationDelayMs() when the persisted field exists (electron/ipc/register.ts:651). With the default setting, the UI restores 60s while the backend silently uses 30s. Persist the default value or align the backend default.

4. The static-analysis coverage regresses. no-inner-html-without-sanitize now scans only **/electron/** (.semgrep/electron-security.yml:10), dropping renderer coverage even though renderer innerHTML assignments still exist, e.g. src/components/PlanViewerDialog.tsx:117. The token URL rule also excludes all of electron/remote/server.ts (.semgrep/ipc-auth.yml:14), which is the file where token-bearing URLs are constructed. Narrow exclusions would preserve the intended protection without muting the highest-risk locations.

[brooksc]

Thank you for the continued thorough review! All four issues are addressed in the latest push.

---

1. Orphaned sub-tasks still lose the signal_done path

Coordinator now tracks orphaned child task IDs in a private orphanedTaskIds = new Set<string>(). In deregisterCoordinator(), each child is added to the set as it's marked orphaned. The HTTP server shutdown in register.ts (MCP_CoordinatorDeregistered) now gates on !coordinator?.hasOrphanedTasks() — if any children are still alive, the server stays up.

When the last orphaned task is closed, a setAllOrphanedDoneCallback fires the same shutdown logic as a deferred operation. Both cleanup paths are covered: cleanupTask() (the MCP close_task path) and removeCoordinatedTask() (the UI close path) both drain orphanedTaskIds and trigger the callback when the set empties.

2. Sub-task agent args are still partly Claude-specific

--mcp-config is now handled the same way as skip-permission flags. AgentDef gains an optional mcp_config_flag?: string field, set to '--mcp-config' for claude-code only (all other agents leave it absent). A new getMcpConfigArgs(command, configPath) helper mirrors getSkipPermissionsArgs() — it normalises via path.basename() and returns [flag, configPath] for Claude or [] for everything else. createTask() spreads the result instead of hardcoding ['--mcp-config', ...].

3. The coordinator notification delay default does not round-trip to the backend

Backend default changed from 30_000 to 60_000 to match the renderer (src/store/core.ts). saveState() already skips persisting the value when it equals 60_000, so with aligned defaults the load path correctly calls setNotificationDelayMs() whenever a non-default value was explicitly set, and both sides agree on the default otherwise.

4. The static-analysis coverage regresses

no-inner-html-without-sanitize now includes **/src/** alongside **/electron/**, so renderer innerHTML assignments are scanned. A pattern-not-either clause excludes DOMPurify.sanitize(...) and the mermaid SVG variable (el.innerHTML = svg); the mermaid line in PlanViewerDialog.tsx also carries an inline // semgrep-disable-next-line comment with rationale. token-embedded-in-url-template drops the whole-file exclusion of electron/remote/server.ts and replaces it with pattern-not: \$PREFIX?token=${mobileToken}$SUFFIX`` — coordinator and other tokens in URL templates will now be flagged, while the three known-safe mobile QR URL constructions are explicitly allowed by variable name.

[johannesjo]

Thank you very much!

Independent multi-agent review — range 2465ab6..b43a427

Reviewed this exact delta with 6 parallel reviewers (correctness, security, architecture, alternatives, performance, simplicity), each verified against b43a427 source. Summary below distinguishes confirmed unfixed in this range (overlaps @johannesjo's open items) from new findings. Note: this is the range only — anything pushed after b43a427 is out of scope here and unverified.

🔴 Critical

C1 — Orphaned signal_done is dead end-to-end (issue #1, half-fixed). electron/mcp/coordinator.ts:1361-1396 + electron/ipc/register.ts:1185-1198
deregisterCoordinator() now correctly retains the in-memory task record. But MCP_CoordinatorDeregistered still does await remoteServer.stop(); remoteServer = null when the last coordinator exits (\!hasActiveCoordinator() && remoteServerStartedForMcp && \!remoteServerRequestedManually). signal_done travels over that HTTP server (POST /api/tasks/:id/done), so a still-running orphan's later signal_done gets ECONNREFUSED before signalDone() is reached — the retained record is unreachable. Same path also unlinkSyncs + nulls each child mcpConfigPath (coordinator.ts:1388-1394), severing lazy/restarted sub-task config. The task-retention half is fixed; the transport/config half @johannesjo flagged is not. Fix: keep MCP transport+config alive until all orphans close, or terminate children explicitly rather than promising resolution.

🟠 Warnings

W1 — bindHost fail-open default flip (new). electron/remote/server.ts:875 — opts.host ?? '127.0.0.1' → opts.host ?? '0.0.0.0'. Flagged by 5/6 reviewers. All current callers pass host explicitly, so no live exposure in this range — but a security-sensitive default was flipped safe→exposed, it contradicts the in-code comment at register.ts:1091 ("uses 127.0.0.1 by default"), and any future host-omitting caller both LAN-exposes the SPA/WS/REST API and defeats the bindHost==='127.0.0.1' MCP-only gate (register.ts:1072,1123). Keep loopback default; require explicit '0.0.0.0'.

W2 — semgrep renderer XSS coverage zeroed (issue #4a, confirmed). .semgrep/electron-security.yml:33 **/src/**→**/electron/**. No innerHTML/outerHTML/insertAdjacentHTML exists under electron/ → rule now matches nothing; 5 live renderer sinks unscanned (PlanViewerDialog.tsx:117,282, ScrollingDiffView.tsx:230, TaskAITerminal.tsx:716, TaskNotesBody.tsx:269). Use include: ['**/src/**','**/electron/**'].

W3 — --mcp-config hardcoded for all sub-task agents (issue #2, partially unfixed). electron/mcp/coordinator.ts:638. skip-perms is now agent-aware via getSkipPermissionsArgs(), but ['--mcp-config', subTaskMcpConfigPath] is appended for every agent. codex/gemini/opencode/copilot get an unknown flag → fail to spawn or never get MCP (can't signal_done).

W4 — getSkipPermissionsArgs divergent source of truth (new). electron/ipc/agents.ts:85-93. Matches only static DEFAULT_AGENTS — custom agents (resolved correctly elsewhere at TaskAITerminal.tsx:592) silently get []. Also returns the live internal DEFAULT_AGENTS array by reference (latent aliasing), and exact path.basename match misses aliased/case-variant commands. Thread AgentDef.skip_permissions_args through spawnDefaults like command/args; return a copy.

W5 — Notification-delay default mismatch (issue #3, confirmed). Backend private notificationDelayMs = 30_000 (coordinator.ts:90); renderer default 60_000, omitted from persist when default; register.ts:651-653 only calls setNotificationDelayMs() when the field is a finite number → omitted → backend stays 30s. Default-config user: UI 60s / backend 30s. Single shared constant, or persist unconditionally.

W6 — Non-atomic rebind teardown (new). electron/ipc/register.ts:1059-1067. MCP→public rebind does stop(); remoteServer=null; remoteServerStartedForMcp=false before await startRemoteServer(...). If the new start rejects (e.g. EADDRINUSE), the exception propagates with remoteServer=null — coordinator MCP transport permanently destroyed, no recovery. Start-then-swap, or restore state on failure.

W7 — Coordinator persistence block duplicated 4× (new). src/store/persistence.ts:173-182, 229-238, 632-644, 732-744. Save active/collapsed byte-identical; load active/collapsed byte-identical. File ~740 lines. A future field forgotten in one of 4 sites = silent round-trip loss with no compiler enforcement. Extract serializeCoordinatorFields/restoreCoordinatorFields (file already uses this pattern for agent defs).

🟡 Suggestions

• .semgrep/ipc-auth.yml:14 whole-file exclude of electron/remote/server.ts blinds the token-in-URL rule in the one file that builds token URLs (issue #4b). Use line-scoped // nosemgrep or a pattern-not for the mobileToken case instead.
• bindHost==='127.0.0.1' string sentinel as deployment-mode flag (3-reviewer consensus) coexists with the remoteServerStartedForMcp boolean — two parallel models, brittle to ::1/localhost. Prefer an explicit mode: 'mcp-only' | 'remote' on RemoteServer.
• Mobile WS read-only is a deny-list (server.ts:782-787) — complete for current message types but a future mutating type is implicitly mobile-allowed. Harden to allow-list (default: ws.close(4003)).
• clientTokenTypes Map + authenticatedClients Set are redundant (lock-step at 4 sites); unavailableReason:'coordinator_active' hardcoded for any loopback bind (register.ts:1076); sendInput is now a dead export in src/remote/ws.ts (not in changed files — cleanup incomplete).

Verified correct (no action)

atomic.ts change is a genuine fix (writeSync→writeFileSync(fd) removes a short-write bug; openSync(tmp,'w',mode)+fchmod yields exact perms). AgentDetail.tsx write-UI removal is clean — no dead code, onCleanup still used. No clientTokenTypes leak. No new mobile info-exposure (coordinator/subtask tokens & task routes correctly 403).

Verdict

Needs changes for this range: C1 + W1 + W2/W3/W5 (the latter three = your open issues 4a/2/3, unfixed at b43a427). If a later push addresses these, re-review that delta to confirm C1's transport half and W1 are actually closed — not just the in-memory/skip-perms halves.

_{🤖 Multi-agent review (6 Claude reviewers; Codex unavailable — sandbox).}

[brooksc]

Thank you for the detailed multi-agent review. The branch has been squashed and pushed with the fixes below, following the same ordering as the latest review comment.

Critical

C1 - Orphaned signal_done is dead end-to-end

Addressed. Coordinator now tracks orphaned child task IDs and preserves child task records plus mcpConfigPath when a coordinator deregisters. The shared MCP HTTP server shutdown path now checks !coordinator.hasOrphanedTasks() before stopping, so still-running orphaned children keep their POST /api/tasks/:id/done path alive. When the last orphan is closed through either the MCP cleanup path or UI close path, the orphan set drains and the deferred server shutdown callback runs.

Warnings

W1 - bindHost fail-open default flip

Addressed. startRemoteServer() defaults to 127.0.0.1, and callers now pass an explicit mode: 'mcp-only' | 'remote'. Manual remote access uses explicit remote mode; coordinator MCP mode uses explicit MCP-only mode.

W2 - Semgrep renderer XSS coverage zeroed

Addressed. .semgrep/electron-security.yml includes both **/electron/** and **/src/** for the innerHTML rule. Known sanitized/intentional cases are narrowly excluded instead of dropping renderer coverage.

W3 - --mcp-config hardcoded for all sub-task agents

Addressed. MCP config injection is now agent-specific. Agent metadata exposes mcp_config_flag, and coordinator spawn defaults can carry the configured flag. Sub-task creation uses that metadata instead of always appending Claude-style --mcp-config.

W4 - getSkipPermissionsArgs divergent source of truth

Addressed. Skip-permission args are copied from agent metadata and can be threaded through coordinator spawn defaults via agentSkipPermissionsArgs. The getter returns copies rather than internal arrays, and command lookup handles path-qualified executables.

W5 - Notification-delay default mismatch

Addressed. Backend coordinator notification delay now defaults to 60_000, matching the renderer/store default. Persisted non-default values are retained and applied after async coordinator initialization.

W6 - Non-atomic rebind teardown

Addressed. Remote rebind now starts the replacement server before stopping the existing server. If replacement startup fails, the current MCP transport remains intact.

W7 - Coordinator persistence block duplicated 4x

Addressed. Coordinator persistence now uses shared serializeCoordinatorFields() and restoreCoordinatorFields() helpers for active and collapsed task save/load paths, reducing drift risk for future coordinator fields.

Suggestions

• Replaced the bindHost === '127.0.0.1' sentinel with explicit remote server mode.
• Hardened mobile WebSocket access from a mutation deny-list to a read-only allow-list for subscribe and unsubscribe.
• Collapsed redundant WebSocket auth tracking into token-type tracking.
• Removed dead remote write exports from src/remote/ws.ts.
• Added Semgrep rule fixture coverage through npm run test:security-rules.

Additional fixes included

• Sidebar now renders coordinated child tasks under their coordinator and includes them in keyboard/navigation order.
• StartMCPServer cleans up partial coordinator state if later setup fails.
• Mobile WebSocket read-only behavior is covered by tests.
• Remote PTY WebSocket output now has bounded backpressure handling.
• OpenSpec proposal/spec/design were cleaned up so the proposal stays WHY-focused and specs describe observable behavior.

Verification

• npm run check passed during commit and push hooks.
• Focused coordinator Vitest suite passed locally before squash.
• npm run lint:security passed.
• npm run test:security-rules passed.
• git diff --check passed.

Known limitation: npx openspec validate --all --strict still fails locally with npm error could not determine executable to run, which appears to be the existing OpenSpec executable/tooling issue rather than a spec validation result.

[johannesjo]

Thank you very much! We're getting closer!