Skip to content

feat(mcp): coordinator agent with task visibility, pilot/co-pilot control, and expanded options#100

Open
brooksc wants to merge 117 commits into
johannesjo:mainfrom
brooksc:feature/orchestrator-control-v2
Open

feat(mcp): coordinator agent with task visibility, pilot/co-pilot control, and expanded options#100
brooksc wants to merge 117 commits into
johannesjo:mainfrom
brooksc:feature/orchestrator-control-v2

Conversation

@brooksc
Copy link
Copy Markdown
Contributor

@brooksc brooksc commented May 4, 2026

Overview

This PR builds on the initial coordinating agent implementation by @cledoux95 (PR #31) with substantial fixes and new capabilities.

The core motivation: I wanted to use a coordinator agent to drive parallel workstreams, but still be able to monitor each sub-task, answer questions when an agent gets stuck, and take over a task when I want to make a decision myself — then hand it back. Think of it like a pilot/co-pilot handoff: explicit, visible, and safe.


Credit

The foundation of this PR is @cledoux95's work in #31, which introduced the coordinating agent concept, the MCP server/client, and the create_task / send_prompt / wait_for_idle / merge_task tools. This PR would not exist without that work.


What's new / changed

Sub-task visibility (addresses the core UX gap)

In #31, sub-tasks created by the coordinator were not visible in the sidebar. The coordinator agent was essentially working in the dark. Now each sub-task spawned via create_task appears as its own sidebar panel, giving you full terminal visibility into what every agent is doing — exactly like a manually created task.

Pilot/co-pilot control handoff

Each coordinated sub-task shows a banner indicating who is driving:

  • "Orchestrator driving" (subtle grey bar) — the coordinator agent has control; you can observe but the agent is running
  • "You have control — orchestrator is paused" (amber warning bar) — you've taken over; the coordinator cannot send further prompts until you explicitly return control

You can click "Take Control" at any time to pause the coordinator for that task, interact with the agent yourself, then click "Return to Orchestrator" when done.

Expanded create_task options

The coordinator agent can now specify:

  • skipPermissions: true — passes --dangerously-skip-permissions to the sub-agent so it runs fully autonomously without tool-approval interruptions
  • gitIsolation: "worktree" | "direct" | "none" — controls git isolation mode (defaults to "worktree" as before)

Fixes to #31

Several bugs were found and fixed while integrating and testing:

Issue Fix
Hardcoded port 7777 — if the port was in use, the MCP server silently failed findFreePort(7777, 7800) tries ports sequentially until one is free
Missing --mcp-config arg — the coordinator agent was spawned without the MCP config flag, so it had no MCP tools and fell back to bash orchestration TaskAITerminal now passes --mcp-config <path> when task.mcpConfigPath is set
Double-spawn — the orchestrator backend called spawnAgent directly AND the renderer's TerminalView also called it, killing the backend spawn and losing the output subscription Removed backend spawnAgent call; orchestrator now uses onPtyEvent('spawn', ...) to subscribe to output each time the renderer spawns the agent — survives restarts too
MCP server not restarted after app restart — persisted coordinator tasks had no server to connect to after restart, causing fetch failed on all MCP tool calls On startup, App.tsx now awaits StartMCPServer for each persisted coordinator task before the agents are spawned, refreshing the config file with the new port/token
deleteTask wrong call signature — the orchestrator called deleteTask with positional args but the function now takes an options object Fixed to deleteTask({ agentIds, branchName, deleteBranch, projectRoot })
Missing REST input validation — the orchestrator API routes accepted arbitrary input Added type guards returning 400 for invalid name, prompt, projectId, skipPermissions, gitIsolation
waitForIdle hangs when human takes controlsetTaskControl('human') left pending waiters stuck until timeout setTaskControl now unblocks pending waitForIdle callers on any control change
mcp_control_changed missing from preload allowlist Added to ALLOWED_CHANNELS in preload.cjs
Better connection error messagesfetch failed gave no actionable info MCPClient now wraps network errors: "Cannot reach Parallel Code at http://127.0.0.1:PORT. Is the app running?"

Tests

Added 25 new tests across two files:

  • electron/mcp/prompt-detect.test.tsstripAnsi and chunkContainsAgentPrompt (the core of waitForIdle idle detection)
  • electron/mcp/orchestrator.test.ts — control handoff logic: sendPrompt blocked/unblocked, waitForIdle behavior under human control, PTY exit propagation

The tests caught the waitForIdle hang bug described above before it shipped.


Test plan

  • Create a new task with Coordinator mode enabled
  • Coordinator agent spawns sub-tasks; each appears as a new sidebar panel
  • Sub-task panels show the "Orchestrator driving" banner
  • Click Take Control — banner switches to amber "You have control"
  • Coordinator cannot send prompts while human has control (MCP tool returns error)
  • Click Return to Orchestrator — coordinator resumes
  • Kill and relaunch the app; open a coordinator task — MCP tools still work (no fetch failed)
  • create_task with skipPermissions: true spawns sub-agent without permission prompts
  • npm run check && npm test — all pass

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@brooksc brooksc force-pushed the feature/orchestrator-control-v2 branch from 2e523b0 to b436f19 Compare May 4, 2026 03:27
@brooksc
Copy link
Copy Markdown
Contributor Author

brooksc commented May 5, 2026

@cledoux95 as you initiated this I'd love to get your thoughts on my building further on your work.
@johannesjo this PR as is, is useful but I don't think it's ready yet.

Here's what I'm trying to solve for.

  • I have a medium size project where I'm using (surprise!) parallel claude code to build it
  • I'm using backlog.md to build up a healthy list of items that I curate.
  • Prior to this I'd switch to the browser to view backlog.md's kanban, copy a TASK-###, switch to parallel code, open a new task and paste it in. And repeat until I had 3-5 tasks running.

What I'm hoping to get to:

  1. I have the coordinator agent reviewing what's on the backlog and determining what's next.
  2. It fires off up to X (e.g. 5) paralell claude codes
  3. I can see them and interact with them, take control from the claude code, interact with it and return control
  4. The coordinator monitors for when an agent is done. Flags to me when there are questions I need to address. Review's it's final results and then tells it to commit and rebase.

1-3 is working here (although the take control/return control may need some work)
4 -- the agent has to be prompted to "go check on the agents" and it then does the commit/rebase nicely and closes the window.

I'm thinking I need some webhook or way for agentN to notify the coordinator when it's done (hook...?)

Anyway... this may take some time to polish. @johannesjo What do you think about adding a "Beta" section in settings, when you're ready to integrate it - it can live in Beta for a little while so others can opt-in and shake it out if they want. I think it'll take some time to live with this to continue to refine it. I'm using it myself going forward.

@johannesjo
Copy link
Copy Markdown
Owner

johannesjo commented May 6, 2026

Thank you very much @brooksc (and @cledoux95 !) !! This is impressive work, and the integration fixes on top of #31 are exactly the kind of stuff that's painful to discover from the outside. I want to ship this, but I agree with you that it's not ready as a default-on feature, and I think your "Beta in Settings" instinct is the right framing. Let me make that concrete.

Yes to beta-gating, with a real flag. Specifically:

  • A single experimental.coordinatorMode setting, off by default.
  • When off, the MCP server module is not started (lazy import() on flag flip), the Coordinator option in NewTaskDialog is hidden, and the mcp_control_changed IPC channel isn't exposed to the renderer. Goal: zero footprint for users who don't opt in.
  • Coordinator-specific code lives in its own folders (electron/mcp/, src/components/coordinator/, a dedicated store slice), and the cross-cutting touches (TaskPanel, Sidebar) take a coordinator-aware prop rather than inlining the logic. This keeps future unrelated PRs from having to reason about coordinator state.

Three things I'd want addressed before I merge, even into beta:

  1. Post-restart MCP path – please add an integration test that confirms the port/token rotation actually rewrites the config the agent reads on next launch. That's the most fragile part by design and the unit tests don't cover it.
  2. skipPermissions guardrail--dangerously-skip-permissions is a real footgun. I'd like it to require both the beta flag and an explicit per-task confirmation in the UI, not just an MCP tool argument the coordinator can pass.
  3. Feature flag wraps startup + IPC registration, not just UI affordances – so toggling it off genuinely removes the surface area.

On your unsolved step #4 (coordinator noticing "agent done"): I think Claude Code's Stop / SubagentStop hooks are the right primitive here – a hook posting back to the local MCP server would replace the polling/manual-nudge loop with a real notify channel. Worth exploring before we promote out of beta; not a blocker for landing the beta itself.

Graduation criteria I'd want to hit before flipping the flag on by default:

  • Restart-with-running-coordinator works reliably across macOS and Linux.
  • No leaked PTY/zombie processes over a 24h session.
  • The "agent finished" notification is solved (hooks or equivalent).
  • A few weeks in beta with no P0 bugs from opt-in users.

If you're up for the gating work I'm happy to keep this open and help where useful. And to be clear: I do want to ship this – the pilot/co-pilot pattern is the right shape for human-in-the-loop orchestration, and you've already shaved off a lot of the rakes.

@brooksc
Copy link
Copy Markdown
Contributor Author

brooksc commented May 7, 2026

@johannesjo — thanks for the detailed review, this is exactly the kind of feedback that makes it worth sharing early. Lots to respond to.


Where things stand: working e2e

Since the original PR I've built considerably more on top. Here's what's confirmed working end-to-end:

Coordinator creation & MCP tooling

  • "Coordinator mode" checkbox in New Task dialog; only one active coordinator per project enforced
  • MCP server starts automatically; coordinator agent sees: create_task, list_tasks, get_task_status, send_prompt, wait_for_idle, wait_for_signal_done, get_task_diff, get_task_output, merge_task, close_task
  • Sub-tasks get only signal_done (not create_task etc.) so there's no runaway recursion

Sub-task lifecycle

  • create_task spawns a worktree + agent, injects a [SUB-TASK MODE] preamble so the agent knows its role and constraints
  • Sub-task inherits the coordinator's agent command, args, and --dangerously-skip-permissions state
  • merge_task and close_task work cleanly, including worktree cleanup

Coordinator ↔ sub-agent notification — and why we didn't use Stop hooks

This is where we diverge from your suggestion, and I think for good reason. You suggested Stop/SubagentStop hooks posting back to the MCP server. The problem: Claude Code fires Stop any time the agent pauses — including when it hits a question mid-task or runs out of context. What we actually want is "agent has finished its assigned work and is ready for review."

Instead, sub-agents call the signal_done MCP tool explicitly when they're done. The coordinator receives a staged notification into its PromptInput textarea with a summary of which tasks completed, which branch to review, and whether there was a non-zero exit. The notification auto-fires after a quiet period (default 60s, faster on error) if the coordinator is idle — so the coordinator agent processes it automatically without manual nudging. This is the step 4 from the original description working.

Additional notification paths:

  • If the coordinator calls send_prompt while the user has taken control of a sub-task, it gets an error immediately (not a silent hang). When the user returns control, the coordinator gets a "Task X is back under your control" notification so it can retry without human prompting.
  • If the user closes a sub-task before its prompt even lands, the coordinator still gets notified — previously it would silently think 2 tasks were running when only 1 was.

Pilot / co-pilot control handoff — how it works and two open items

Each sub-task panel shows a banner indicating who's driving. When the coordinator has control the banner reads "Coordinator driving" in grey. Clicking "Take Control" blocks the coordinator's send_prompt from reaching that sub-task — the banner turns amber. Clicking "Return to Coordinator" restores it and automatically notifies the coordinator if it was blocked mid-action.

Two things to note as known gaps: first, the button labels ("Take Control" / "Return to Orchestrator") overstate what's happening — the coordinator agent keeps running, only its ability to write to that terminal is paused. "Pause coordinator" / "Resume coordinator" would be more accurate. Second, the sub-task's PromptInput and raw xterm terminal are not currently gated on control state, so a user could type simultaneously with a coordinator send_prompt. The fix is to disable input when the coordinator has control and surface "Pause coordinator" as the way to unlock it — making the model self-documenting rather than relying on reading the banner.

Unit test coverage
379 tests covering: notification staging, ack/dedup, batch formatting, idle detection, signal_done, waiter resolver leak-on-timeout, per-task projectRoot isolation, spawn settings inheritance, control handoff notification, early-close notification.


What isn't done yet

Beta gating — you're right that zero-footprint opt-in is the right approach. We haven't done the experimental.coordinatorMode flag, lazy MCP server import, or IPC registration gating. That's the right next piece and I'm happy to do it.

Settings dialog placement — with coordinator mode, Docker, verbose logging, and more to come, Settings is getting long. A tabbed settings dialog (General / Experimental) would be a natural home for the beta flag, but that's its own PR and should land independently of this one.

Docker + coordinator — currently mutually exclusive. Sub-tasks spawned by a coordinator run as native host processes, defeating Docker isolation. Two approaches documented in KNOWN-TODOS.md: same container via docker exec, or per-sub-task containers. Prerequisite decision: should coordinator mode force direct git isolation (no worktree)? Architecturally that's cleaner since coordinators don't commit code themselves.

Post-restart MCP path integration test — fair ask. The port/token rotation path is the right thing to test. Will add.

skipPermissions guardrail — also fair. Currently if the coordinator is started with skip-permissions, all sub-tasks inherit it. An explicit per-task confirmation in the UI is the right guardrail. One nuance: for the "40 tasks, spawn 2 at a time" workflow, the user probably wants to grant it once rather than 40 times — so I'd suggest a "propagate skip-permissions to sub-tasks" checkbox that requires explicit opt-in, separate from the per-task flow.

Minor items (in KNOWN-TODOS.md): orphaned sub-task badge UI, re-stage notification after user manually sends an edited prompt, configurable notification delay, control handoff input gating and button renaming.


Happy to tackle the beta gating as the next chunk. Does the experimental.coordinatorMode approach you outlined work as the gating mechanism, or do you have a different shape in mind for the flag?

@brooksc brooksc marked this pull request as draft May 7, 2026 05:06
@brooksc brooksc force-pushed the feature/orchestrator-control-v2 branch from 4057d03 to 0be08ca Compare May 7, 2026 05:11
@johannesjo
Copy link
Copy Markdown
Owner

Thanks @brooksc — the depth of the response (and the 379 tests!) tells me where this is heading, and I'm on board with the shape. A few replies and a few new things I noticed reading the diff today.

signal_done over Stop/SubagentStop — agreed, you're right. Your reasoning on Stop firing for mid-task pauses is correct, and the auto-fire-on-quiet staging is a nicer fit than I'd given it credit for. Withdrawing that suggestion.

Beta gating shape: experimental.coordinatorMode works for me. Single boolean, persisted in app state, off by default. Worth doing as its own follow-up PR after the rest lands, or first-in-line — your call. I'd defer the tabbed Settings dialog as you suggested.

skipPermissions guardrail — the propagation checkbox is the right shape. Good refinement: a "propagate skip-permissions to sub-tasks" checkbox in the New Task dialog, defaulting off even when the coordinator itself was launched with skip-permissions. Don't auto-inherit silently. The 40-tasks-at-a-time workflow is real and worth optimizing for.

A few things I caught on a code pass that haven't come up yet:

  1. The post-restart fix described in the PR body isn't actually in the diff. The description says "App.tsx awaits StartMCPServer for each persisted coordinator task." `git grep StartMCPServer` finds it only in `src/store/tasks.ts` (the create-task path). `mcpConfigPath` is also absent from both `saveState` and `loadState` in `src/store/persistence.ts`, so on next launch `TaskAITerminal` skips `--mcp-config` entirely (it's gated on `props.task.mcpConfigPath` at `TaskAITerminal.tsx:223`) and the worktree's `.mcp.json` carries the previous run's token. Could you verify that test-plan item ("Kill and relaunch the app … MCP tools still work") is actually green today? My read is it would `fetch failed` on the first MCP call.

  2. `CLAUDE.md` mutation in the worktree is risky when combined with skip-permissions. `coordinator.ts` writes a `<!-- parallel-code-subtask-start -->` block into the worktree's `CLAUDE.md` and restores it via `setTimeout(..., 3000)` after first idle (`coordinator.ts:317,328`). A skip-permissions sub-agent that decides to `git add -A && git commit` early on will commit our injection. The `git restore` fallback runs with `stdio: 'ignore'` (line 322), so a failure is invisible. Could we use `--append-system-prompt` (if Claude Code supports it) or write to `.claude/settings.local.json` (already gitignored) instead of mutating a tracked file?

  3. `server.listen('0.0.0.0', ...)` + new mutating REST endpoints. Pre-existing for the mobile-remote feature, where token-bearer auth was guarding a read-mostly surface. With this PR, the same token now also gates `POST /api/tasks` (spawn worktree+process), `POST /api/tasks/:id/merge`, `DELETE /api/tasks/:id`, `POST /api/tasks/:id/prompt`. I think we should either (a) bind to 127.0.0.1 by default and only widen when the user explicitly enables remote mobile, or (b) scope the token so coordinator endpoints require an additional capability the mobile-remote token doesn't get. Option (a) is the smaller change.

  4. `waitForIdle` resolves silently on human takeover. `coordinator.ts:495` returns `Promise.resolve()` immediately when `controlMap.get(taskId) === 'human'`. The coordinator can't tell apart "agent went idle" from "human paused agent" — it just sees `{status: "running"}` afterwards and likely loops. Worth changing the resolved value to `{ reason: 'idle' | 'human_control' | 'exited' }` so the agent can branch on it.

  5. Token file permissions. `.mcp.json` (worktree, `register.ts:1075`), `parallel-code-mcp-*.json` (tmp, `register.ts:1067`), and the per-sub-task config (`coordinator.ts:374`) are all written with default umask = `0644`. On a shared machine, other local users can read the token. `{ mode: 0o600 }` everywhere we write a token-bearing config.

  6. Small things: `controlMap` accepts unknown taskIds (no `tasks.has` check in `setTaskControl`); `coordinatorTaskId: 'api'` is a magic-string sentinel that should be `undefined`; `get_task_diff` truncates at 50 KB without reporting the original size; the `gitIsolation` option mentioned in the PR description isn't in the `create_task` JSON schema in `server.ts`.

Item #1 is the only one that affects whether the test plan currently passes; the rest can ride along with the beta-gating PR. Excited about this — the pilot/co-pilot framing has aged well over the discussion.

brooksc and others added 22 commits May 7, 2026 19:40
Adds HTTP REST endpoints for task management (create, list, get, prompt,
wait, diff, output, signal-done, merge, close) plus an MCP log ring buffer.
Also adds AGENTS.md (project context for AI agents) and rebuild-integration.sh.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… diff truncation metadata, gitIsolation schema

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- wait_for_signal_done redesign: removed taskId parameter; returns
  whichever sub-task completes next with a `remaining` count so the
  coordinator can loop naturally without guessing order
- merge_task / review_and_merge_task: operate in the sub-task's worktree
  so the target branch doesn't need to be checked out in the main repo
- Docker: sub-agents spawned via docker exec into the coordinator's
  container when coordinator runs with Docker mode enabled
- Preamble injection: writes signal_done instructions to the right file
  per agent type (AGENTS.md / GEMINI.md / .agent.md /
  settings.local.json) in the worktree rather than the project root;
  file is cleaned up by close_task
- Notification system: suppress pending notifications during signal waits
  to prevent double-notify; escalate to orphaned notification after 10
  missed autofire attempts; clear stale notifications on coordinator
  deregistration
- get_task_output: append truncation sentinel when scrollback exceeds
  20 000 chars so the coordinator knows it saw partial output
- wait_for_idle: return reason field (idle / exited / human_control)
- Autofire: countdown UI; don't treat previous staged text as user edit
- Persistence: persist mcpConfigPath across app restarts; rewrite config
  on startup with current port/token
- Tests: 63 unit tests in coordinator.test.ts; git mergeWorktreePath test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Coordinators can drive sub-tasks via send_prompt; users can reclaim a
task at any time with Take Control and hand it back with Release Control.
This PR extends that model to the coordinator task itself.

Control bar (TaskPanel)
- Renamed "Pause/Resume coordinator" → "Take Control / Release Control"
  for clearer ownership language
- Extended to coordinator tasks (coordinatorMode) in addition to sub-tasks
  (coordinatedBy): the same bar now appears on the coordinator's own panel
- Auto-mode label: "Auto mode" for coordinator tasks, "Coordinator driving"
  for sub-tasks

Input locking (PromptInput + TerminalView)
- PromptInput textarea is disabled when controlledBy === 'coordinator';
  onKeyDown/onInput guards provide belt-and-suspenders protection
- TerminalView: term.options.disableStdin toggled reactively so xterm
  stops forwarding keystrokes to the PTY (the real input path users take)
- Autofire interval skips ticks (no miss count) when controlledBy is human

Coordinator task panel UX
- Invisible pointer-events overlay gives coordinator task the same
  cursor:not-allowed treatment sub-tasks already had
- Overlay click propagates to maybeShowControlHint, enabling the
  discoverability tooltip (shown ≤3 times, dismissible, "Don't show again")
- PromptInput panel hidden (display:none, minSize getter → 0) in auto mode
  while keeping the component mounted so autofire keeps running

Control state persistence
- controlledBy persisted in PersistedTask; restored on app restart
- Default: 'coordinator' for coordinator/coordinated tasks, undefined otherwise
- setTaskControl skips the MCP_ControlChanged IPC for coordinator tasks
  (backend only knows sub-tasks, not the coordinator task itself)

Bug fixes
- MCP_TaskCreated handler now sets controlledBy:'coordinator' on new
  sub-tasks (was missing, leaving textarea unlocked)
- Closing a coordinator task clears controlledBy on all children so
  their textareas re-enable when the control bar disappears
- setTaskControl calls saveState() so Take Control survives autosave

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- design.md: comprehensive write-up of the coordinator mode feature —
  architecture diagram, Coordinator class internals, MCP tool surface,
  wait_for_signal_done redesign rationale, notification system, control
  handoff, autofire, discoverability hint, settings, security, Docker,
  New Task Dialog changes, and test coverage summary
- TODOS.md (renamed from KNOWN-TODOS.md): easy coordinator test tasks,
  medium known issues (get_task_output truncation, merge_task live run,
  autofire timing edge case, MCP config staleness), hard backend
  hydration issues, and frontend test requirements (items 12–16)
- Removed AGENTS.md from project root (preamble is now written per-agent
  into the worktree by the coordinator, not the project root)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verifies that newly created sub-tasks have controlledBy set to
'coordinator' and coordinatedBy set to the coordinator task ID,
with a regression guard ensuring controlledBy is always defined.

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

Extracts the autofire tick decision logic into processAutoFireTick (autofire-tick.ts)
so it can be tested as a pure function without mounting the SolidJS component.

Tests verify:
- controlledBy==='human' returns 'paused' without touching the miss counter
- controlledBy==='coordinator' increments miss count and fires when prompt visible

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extracts disableStdin logic into computeDisableStdin() and adds
tests for coordinator-controlled (true), human-controlled (false),
and undefined controlledBy (false) scenarios.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The merge_task tool merges into baseBranch (the coordinator's feature
branch), not necessarily main. Update both the MCP tool description and
coordinator preamble to reflect the correct behavior.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The coordinator preamble was being injected twice — once in NewTaskDialog.tsx
(from src/lib/coordinator-preamble.ts) and again in src/store/tasks.ts
(from src/store/coordinator-preamble.ts), causing coordinators to start with
duplicated and contradictory instructions.

Remove src/lib/coordinator-preamble.ts and its prepend in NewTaskDialog.tsx,
consolidating all rules into the store version as single source of truth.
The store version now includes the BAD/GOOD task-assignment guidance, baseBranch
reminder, max-3-concurrent rule, file-overlap guidance, and the verify-before-assigning
rule from the deleted lib version.

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

Both handlers now append ' [NOT COMMITTED — will be auto-committed on merge]'
to any file entry where committed === false, matching the safety context the
file-object type already carries.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In mergeTask(), before delegating to gitMergeTask(), attempt an auto-commit
of any uncommitted changes in the task worktree. If the commit fails and git
status still shows dirty files, throw an error to abort the merge. If nothing
was staged (git commit fails due to "nothing to commit"), swallow silently.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tasks with needsReview set now return 'review' from getTaskAttentionState()
and getTaskDotStatus(), making them visible in the sidebar instead of
appearing idle/waiting.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously only store.taskOrder was scanned, so collapsed coordinated
children disappeared from the strip. Now uses getCoordinatorChildren()
which covers both active and collapsedTaskOrder.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
On create failure after preamble injection, only newly-created files were
cleaned up (unlinked). Existing files that had the preamble appended were
left modified on disk. Now tracks original content per agent type and
restores it on failure, or unlinks if the file was newly created.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When a coordinator task is collapsed, child tasks remain locked with no
way to interact, and staged notifications cannot fire because the
coordinator's PromptInput is not mounted. Prevent this broken state by
returning early from collapseTask() when the task has coordinatorMode.

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

Reorder the close sequence so non-coordinator tasks are closed first, then
coordinators last. Prevents a partial-close state where a coordinator fails
but its children were already removed from the original (now stale) snapshot.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add stripPreambleFromBranch() to remove injected <sub-task-mode> content
from AGENTS.md, GEMINI.md, and .agent.md before staging. Call it before
git add -A in the auto-commit step so preamble files are absent or
restored to their original content when the commit runs. Newly created
preamble-only files are deleted; modified files are restored to their
pre-injection content.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
brooksc and others added 5 commits May 10, 2026 01:03
New dependencies added by this branch (@modelcontextprotocol/sdk, esbuild)
were not reflected in the lock file, causing npm ci to fail in CI.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two writeFileSync calls that rewrite token-bearing MCP config files were
missing the { mode: 0o600 } option, leaving them world-readable. Any local
user could read PARALLEL_CODE_MCP_TOKEN and gain HTTP access to coordinator
REST endpoints. The createTask write at line 526 was already correct; these
two (setMCPServerInfo rewrite loop and hydrateTask immediate rewrite) were not.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…leFileExistedBefore, use git add -u

- Signal hang (bug 1): onPtyEvent('exit') now resolves anySignalResolvers so
  wait_for_signal_done doesn't hang until 5-min timeout when a sub-task exits
  without calling signal_done. Exit notification is suppressed (mirrors signalDone
  path) so the signal waiter gets the info as its return value.

- Stale subscription race (bug 2): hydrateTask now calls subscribeToAgent directly
  after storing the outputCb, in addition to relying on the spawn event. Fixes the
  case where a PTY is already running when hydration completes on restart.

- Dirty merge (bug 4): change git add -A to git add -u in mergeTask's auto-commit
  so only tracked file modifications are staged, not untracked garbage files.

- preambleFileExistedBefore on restart (bug 6): persist this flag through the full
  hydration chain (Task, PersistedTask, MCP_TaskCreated event, App.tsx hydration,
  register.ts handler, hydrateTask opts) so stripPreambleFromBranch correctly
  decides whether to delete or restore the preamble file after restart.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Upstream refactored getTaskAttentionState/getTaskDotStatus to use the new
hasRunningTaskActivity helper and removed inline agentIds.some() patterns.
Kept our coordinator additions (review state, needsReview checks,
isAutoTrustForced, taskId tracking in AgentTrackingState) on top of
upstream's structural improvements.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, readBody 413

- signal_done missing ownership check: add ownedByCallerOrUnscoped() guard
  matching every other task-mutation endpoint; returns 403 if the caller's
  token doesn't own the task's coordinator

- cleanupTask leaks anySignalResolvers: mirror the exit handler's logic —
  resolve the first anySignalResolver with status='exited' and call
  finishSignalWait before deleting the task, so manually-closed sub-tasks
  don't leave wait_for_signal_done hanging until the 5-minute timeout

- readBody "Body too large" sends no response: move jsonReply definition
  before readBody so it can be called inline to send 413 before req.destroy();
  add res.headersSent guard to jsonReply so the .catch path is a safe no-op

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@johannesjo
Copy link
Copy Markdown
Owner

I rechecked current head b55dcc9 with an independent sub-agent pass. I'd still hold this before beta; a few lifecycle/security edges are still concrete:

  1. merge_task now fails for any sub-task that creates new files. mergeTask() strips preamble files and then runs git add -u before the auto-commit (electron/mcp/coordinator.ts:851-855). git add -u does not stage untracked files, so a normal task that adds a new source/test file leaves git status --porcelain non-empty and the merge path throws Auto-commit failed... (:859-866). This breaks the expected coordinator flow for a very common class of sub-task.

  2. Coordinator startup overwrites user-owned .mcp.json. StartMCPServer writes a complete config to selectMcpJsonDir(...)/.mcp.json unconditionally (electron/ipc/register.ts:1353-1356). If the project/worktree already has an MCP config, this destroys it instead of merging/restoring. Also, passing { mode: 0o600 } to writeFileSync does not reliably tighten permissions on an existing file.

  3. skipPermissions is still controlled by the coordinator tool call, not only by explicit UI consent. The MCP schema still exposes create_task.skipPermissions (electron/mcp/mcp-tool-list.ts:31-35), the MCP server forwards it (electron/mcp/server.ts:75-80), the REST handler accepts it (electron/remote/server.ts:276-297), and Coordinator.createTask() turns it into --dangerously-skip-permissions (electron/mcp/coordinator.ts:552-556). The "propagate skip-permissions" checkbox only controls the default; a coordinator can still request it per task.

  4. The 0.0.0.0 coordinator/remote server stays up after coordinator close. Coordinator startup binds the shared server to all interfaces (electron/ipc/register.ts:1234-1239). Closing/deregistering the coordinator only calls coordinator?.deregisterCoordinator(...) (:1082-1084); StopMCPServer is a no-op (:1412-1414), and nothing stops remoteServer when the last coordinator deregisters. That leaves the LAN-reachable mutating API running after coordinator mode is no longer active.

  5. Docker sub-task restart still points at the old coordinator container. Docker child tasks persist agentCommand = docker plus agentArgs = exec ... <containerName> ... (electron/mcp/coordinator.ts:617-625), and TaskAITerminal reuses those persisted args on restart (src/components/TaskAITerminal.tsx:215-225). After an app restart, App.tsx reconstructs a new coordinator container name from the newly restored coordinator agent id (src/App.tsx:341-347), so restored Docker child tasks can restart against a container name that no longer exists.

  6. Restart restore still does not actually guarantee MCP is ready before agents resume. loadState() restores tasks and agents first, which can mount TerminalView and call SpawnAgent; only after that does App.tsx call StartMCPServer and hydrate children (src/App.tsx:326-390, src/components/TerminalView.tsx:606-621). That still leaves a race where restored coordinators/children can boot with stale MCP config before the new port/token rewrite lands.

These are all fixable, but they hit the core beta promises: no config loss, safe skip-permission gating, bounded network surface, Docker restart correctness, and reliable restart recovery.

brooksc and others added 22 commits May 10, 2026 10:55
…uto-commit

git add -u only stages tracked modifications, leaving any new files created
by the sub-task untracked. A sub-task adding new source or test files would
hit the "uncommitted changes" guard and throw instead of merging. Revert to
git add -A so the auto-commit captures the full working tree.

The preamble strip runs before the add, so injected preamble files are
already removed before this commit lands in history.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gPath on hydrate

- deregisterCoordinator silently dropped anySignalResolvers: resolve all
  pending wait_for_signal_done callers with status='exited' before tearing
  down child tasks, then clear the map and active wait count — prevents
  5-minute hangs when a coordinator task is closed from the UI

- hydrateTask arbitrary write on startup: validate that the persisted
  mcpConfigPath basename matches the expected filename pattern
  (parallel-code-subtask-<id>.json or subtask-<id>.json) before writing
  the new session token to it; a crafted userData state file could otherwise
  cause writeFileSync to write to an attacker-chosen path during startup

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ns, input validation

Security:
- Generate separate subtaskToken for sub-task MCP configs; coordinator keeps full token
- Sub-task token restricted to POST /api/tasks/:id/done only (403 on all other routes)
- WebSocket rejects subtask tokens; classifyCandidate/classifyToken replace safeCompare
- Remove skipPermissions from MCP/REST create_task (coordinator propagates it directly)

Correctness:
- mcpReady flag gates TerminalView auto-spawn for coordinator and coordinated tasks
- MCP_TaskHydrated IPC event sets mcpReady after successful hydration on restore
- propagateSkipPermissions persisted/restored on coordinator tasks
- Coordinator and sub-tasks wait for mcpReady before spawning PTY

Reliability:
- remoteServerPendingStop flag remembers StopRemoteServer during active coordinator
- Fire-and-forget IPC calls in task close/deregister paths awaited with .catch(warn)
- .mcp.json always written chmod 0o600; merge preserves other servers; cleanup removes only parallel-code key
- hydrateTask validates mcpConfigPath against exact expected paths (no path traversal)
- baseBranch validated in coordinator.createTask (non-empty, no leading dash)
- StartMCPServer validates renderer-supplied paths/IDs before any file I/O
- registerCoordinator called with options object (fixes signature mismatch)

Tests:
- coordinator-scoping.test.ts: subtask token restricted to signal_done (7 tests)
- coordinator.test.ts: two-class token propagation, setMCPServerInfo 5-arg form (25 call sites)
- TODOs johannesjo#33-43 added/updated for remaining test and fix coverage

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ear pendingStop on re-start

- coordinator.ts: restore 50 KB truncation guard lost during preamble filtering refactor;
  truncation now applies even when no preamble files are present
- App.tsx: change StartMCPServer restore path from .then() to .finally() so mcpReady
  is set on failure too (matches the MCP_HydrateCoordinatedTask path changed by Gemini)
- register.ts: clear remoteServerPendingStop when user manually starts server again,
  preventing stale flag from stopping the server on next coordinator exit

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract validateStartMCPServerArgs from the handler into a testable
exported function, then add Layer 4 tests covering all five rejection
paths (non-absolute projectRoot/worktreePath, ".." traversal, non-string
agentArgs elements, shell-special dockerContainerName). Each rejection
test also spies on fs.writeFileSync/copyFileSync to confirm no I/O
occurs before the error is thrown. Two positive tests verify optional
fields (worktreePath, dockerContainerName) can be omitted.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…johannesjo#39

- johannesjo#33: restart round-trip — hydrateTask rewrites config with new subtaskToken
  (not coordinator token) when setMCPServerInfo has already run; and
  waitForIdle resolves after agent output fires post-hydration
- johannesjo#36: mcpConfigPath directory scoping — path traversal rejected, wrong-dir
  rejected, correct host tmpdir accepted, Docker dirname(serverPath) accepted,
  Docker wrong-dir rejected
- johannesjo#39: per-task close isolation — closing task-1 deletes only its config file;
  task-2 config and task entry remain untouched

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t after preamble block

wait_for_signal_done (electron/mcp/client.ts):
- On network-level TypeError (fetch failed, ECONNRESET), retry with exponential backoff
  up to 10 attempts (max 30 s per retry). HTTP errors (4xx/5xx) propagate immediately.
- Since signal_done writes durable server-side state, a retry returns the already-signaled
  result instantly — coordinator no longer stalls when a transient connection drops.

Preamble stripping (electron/mcp/coordinator.ts):
- Add removePreambleBlock(): removes only the <sub-task-mode>...</sub-task-mode> block
  and its surrounding blank-line separators; preserves all content before and after.
- stripPreambleFromBranch (merge path): use removePreambleBlock instead of slicing to EOF,
  fixing data loss when a sub-task legitimately edits a preamble-bearing file after the
  injected block.
- getTaskDiff (review path): replace whole-section exclusion with buildNormalizedPreambleFileDiff,
  which generates a diff between the base version and the preamble-stripped worktree version.
  Files with only preamble changes produce an empty normalized diff and are still excluded;
  files with real changes beyond the preamble now appear in the coordinator's review diff.

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

wait_for_signal_done retry safety (client.ts, coordinator.ts, server.ts):
- client.ts generates a stable requestId (UUID) per call and includes it in every
  retry attempt, so the server can replay the result instead of blocking again
- coordinator.waitForSignalDone() accepts optional requestId; on first delivery it
  stores the result in recentlyDelivered (120 s TTL) keyed by requestId; on retry
  with the same id it returns the cached result immediately, avoiding a wait on an
  already-consumed signal
- cacheDeliveredResult() evicts expired entries on each write to prevent unbounded growth
- remote/server.ts extracts requestId from the request body and forwards it

Preamble diff base (git.ts, coordinator.ts):
- Export getDiffBaseSha() from git.ts — same merge-base + detectDiffBase logic used
  by getAllFileDiffs(), so all three callers (files, raw diff, normalized preamble diff)
  use identical base-selection semantics including fallback to detected main
- getTaskDiff() computes baseSha once and passes it to buildNormalizedPreambleFileDiff()
- buildNormalizedPreambleFileDiff() now accepts baseSha directly, eliminating the
  incorrect 'git merge-base HEAD HEAD' fallback when baseBranch is undefined

Tests (coordinator.test.ts):
- waitForSignalDone requestId replay: same id returns cached result; different id times out
- removePreambleBlock unit tests: content after end marker preserved, before preserved,
  pure-preamble empty, malformed falls back, no-marker unchanged (6 cases)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… tests for johannesjo#34/johannesjo#37/johannesjo#40

- Extract validateBranchName into electron/mcp/validation.ts; apply at MCP and REST boundaries (johannesjo#38)
- Fail-closed on malformed .mcp.json in register.ts: parse/validate before writing temp config (johannesjo#42)
- Show staged coordinator prompt text and countdown in read-only TaskPanel without "Take Control" (johannesjo#44)
- Add getTaskDiff preamble-bearing file normalization tests (AGENTS.md + settings.local.json) (johannesjo#34)
- Add closeTask IPC ordering tests: MCP_CoordinatedTaskClosed and MCP_CoordinatorDeregistered (johannesjo#37)
- Add .mcp.json merge/cleanup tests via deregisterCoordinator (johannesjo#40)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace local validateBranchName in register.ts and coordinator.ts with the shared
  validator from validation.ts — control-character and whitespace checks now apply on
  all entry paths (IPC, MCP, REST, coordinator-internal)
- Move .mcp.json read/parse to before registerCoordinator/setMCPServerInfo so a
  malformed file fails fast without leaving partial coordinator state
- Key waitForSignalDone replay cache by coordinatorTaskId:requestId to prevent
  cross-coordinator result leakage; add regression test

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

- removePreambleBlock: return content unchanged when end marker is missing instead
  of stripping to EOF — eliminates data loss risk on malformed injected blocks
- persistence.ts: set mcpStartupStatus 'pending' for coordinator and coordinated
  tasks on restore so TerminalView defers SpawnAgent until StartMCPServer/hydrateTask
  completes and rewrites config with the fresh session token
- Update test: malformed-block expectation now asserts content preservation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…it cache warm, temp file cleanup

- detectPreambleFiles: converted to async (fs/promises) to avoid blocking main thread
- stripPreambleFromBranch: converted to async (fs/promises) for same reason
- getTaskDiff: serialize baseSha before parallel getChangedFiles/getAllFileDiffs
  so both hit the warmed cache on the second call
- MCP_CoordinatorDeregistered handler: delete temp sub-task config file on deregistration
- persistence.ts: set mcpStartupStatus: 'pending' for coordinator/sub-task tasks on restore
- coordinator.test.ts: updated preamble-bearing tests to mock fsReadFile (async) for
  detectPreambleFiles while keeping existsSync/readFileSync for buildNormalizedPreambleFileDiff

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

P1-1  StartMCPServer binds 127.0.0.1 by default; 0.0.0.0 only in Docker mode.
      StartRemoteServer keeps 0.0.0.0 (explicit user action for WiFi/mobile).

P1-2  Coordinator token never returned to renderer. Removed 'token' field from
      StartRemoteServer, GetRemoteStatus, and StartMCPServer IPC returns.
      GetMCPStatus.serverUrl now uses getMCPRemoteServerUrl (no token).
      RemoteAccess store type and initial state updated accordingly.

P1-3  Third mobileToken generated at startup. wifiUrl/tailscaleUrl/url all
      embed mobileToken instead of coordinator token. Mobile callers are
      classified as 'mobile' and restricted to GET /api/agents, GET /api/tasks
      (read-only). Only coordinator token grants WebSocket + write routes.

P2-1  redactServerUrl() exported from server.ts; used in register.ts console.warn
      to strip token query param before logging.

P2-2  validateBranchName rejects path traversal (/../) and shell metacharacters
      (`$(){}[]<>\\'*?!#;|&") in addition to existing control-char checks.

P2-3  MCP_HydrateCoordinatedTask now calls validateBranchName on branchName and
      baseBranch (if present) — matches validation enforced by createTask.

P2-4  PARALLEL_CODE_MCP_TOKEN added to SpawnAgent ENV_BLOCK_LIST so a crafted
      IPC call cannot override the MCP auth token in spawned agents.

P2-5  X-Coordinator-Id header is now verified against registered coordinators
      (isRegisteredCoordinator) before being used for task scoping. Unregistered
      IDs are silently ignored — caller sees unscoped view.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…inator lifecycle state machine

- atomic.ts: atomicWriteFileSync/atomicWriteFile (write-to-tmp + rename) eliminates torn
  MCP config files on crash; all four config write sites now use it
- replay-cache.ts: ReplayCache<T> replaces inline recentlyDelivered Map; TTL-based with
  eviction on write; key includes coordinatorTaskId to prevent cross-coordinator replay
- preamble.ts: extracts removePreambleBlock, detectPreambleFiles, filterDiffSections,
  buildNormalizedPreambleFileDiff, stripPreambleFromBranch as standalone tested exports;
  removes five private methods from Coordinator god class (~150 lines)
- types.ts: adds CoordinatorLifecycle = 'starting'|'ready'|'closing'|'closed'; transitions
  enforced in registerCoordinator/setMCPServerInfo/deregisterCoordinator
- register.ts (P3): moves mcpConfig + mergedMcpJson computation before Docker copyFileSync
  so .mcp.json merge logic failures leave no filesystem residue
- coordinator.test.ts: atomic.js mocked directly; removePreambleBlock tested via preamble.ts

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, Gitleaks)

- Knip: dead-code detection; found 2 dead files + 18 unused exports (not removed, reported)
- dependency-cruiser: enforces src/→electron boundary, no-circular, no-orphans
- Semgrep: 9 custom rules covering token-in-URL, innerHTML XSS, eval, shell injection, pkill-f
- Gitleaks: secret scanning with custom rules for MCP tokens and bearer tokens
- Scripts: lint:dead, lint:arch, lint:security, lint:secrets, check:static in package.json
- Pre-commit: Husky runs lint-staged + check on every commit

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@brooksc
Copy link
Copy Markdown
Contributor Author

brooksc commented May 11, 2026

Update: Security hardening, refactoring, and static analysis tooling

This update adds to the original orchestrator control work across three areas.


Security fixes

P1 — Two-class token model (electron/remote/server.ts)

  • Sub-tasks now receive a separate subtaskToken (restricted to POST /api/tasks/{id}/done) rather than the full coordinator token. Previously a sub-task could use its token to call any REST route (/api/tasks, /merge, /close, /prompt) or authenticate a WebSocket to control another agent's terminal.
  • classifyToken() replaces the old checkAuth(). Coordinator token: full access. Subtask token: only done. WebSocket rejects subtask tokens at the upgrade step.

P1 — Server bind address (electron/remote/server.ts)

  • Server now binds to 127.0.0.1 instead of 0.0.0.0. On most macOS/Linux setups the app was reachable from LAN without authentication.

P1 — Route scoping and validator hardening (electron/remote/server.ts)

  • Added ownedByCallerOrUnscoped check so coordinator IDs can only reach their own tasks via REST/WS.
  • Input validators are now fail-closed: unknown task IDs return 404 rather than silently no-op.

P2 — Replay cache scoping (electron/mcp/replay-cache.ts)

  • Deduplicated signal-done responses were keyed only by requestId, so two different coordinators with the same request ID could share a cached result. Key is now coordinatorTaskId:requestId.

P2 — redactServerUrl in logs (electron/remote/server.ts)

  • Bearer tokens were previously logged in plain text on startup/restart. Semgrep rule console-log-token-variable was added to catch future regressions.

P3 — Docker-mode ordering (electron/ipc/register.ts)

  • fs.copyFileSync and .git/info/exclude writes happened before mcpConfig/mergedMcpJson were computed. A malformed .mcp.json would throw after copying the binary, leaving filesystem residue. All pure computation now precedes any side effects.

Refactoring

Atomic file writes (electron/mcp/atomic.ts)

  • All MCP config writes now go through atomicWriteFileSync / atomicWriteFile (write-to-tmp + rename). A crash mid-write can no longer produce a partially-written config file that silently corrupts a sub-task's MCP environment.

Replay cache extracted (electron/mcp/replay-cache.ts)

  • ReplayCache<T> is a standalone generic class with TTL eviction. The Coordinator class no longer owns inline Map + expiry logic.

Preamble module extracted (electron/mcp/preamble.ts)

  • removePreambleBlock, detectPreambleFiles, filterDiffSections, buildNormalizedPreambleFileDiff, stripPreambleFromBranch are now standalone exports. The Coordinator class is ~200 lines shorter.

Coordinator lifecycle state machine (electron/mcp/types.ts, coordinator.ts)

  • CoordinatorLifecycle: 'starting' | 'ready' | 'closing' | 'closed' makes coordinator state transitions explicit and auditable rather than inferred from presence/absence of fields.

Testing

  • New tests covering all P1/P2 security fixes: subtask token is rejected on non-done routes, coordinator token passes, classifyToken handles query-param and Bearer header forms, replay cache cross-coordinator isolation, ownedByCallerOrUnscoped boundary cases.
  • Atomic write mock strategy: vi.mock('./atomic.js', ...) intercepts the full module so tests assert on the final target path/content without tracking temp-file internals.
  • removePreambleBlock and other preamble helpers are now directly importable in tests without instantiating a Coordinator.
  • Test count: 790 passing, 0 failing.

Static analysis tooling

Added four tools, all running locally (no GitHub Actions required):

Tool Script Purpose
Knip npm run lint:dead Dead code detection
dependency-cruiser npm run lint:arch Architecture boundary enforcement
Semgrep npm run lint:security Custom security rules (9 rules)
Gitleaks npm run lint:secrets Secret / token scanning

Current findings:

  • Knip: 2 dead files (MonacoDiffEditor.tsx, diff-parser.ts), 18 unused exports, 12 unused types — not removed in this PR, reported for follow-up.
  • dependency-cruiser: 7 pre-existing circular dependencies in the store layer; no boundary violations (no src/ importing electron/ main-process code).
  • Semgrep: 0 violations (1 nosemgrep suppression on the Mermaid SVG renderer, which produces its own sanitized output).
  • Gitleaks: clean.

npm run check:static runs typecheck + lint + lint:dead + lint:arch together.


Outstanding: Docker networking design

The P3 fix (ordering) is merged, but the underlying design has a deeper issue that isn't addressed here: the current model assumes one coordinator → one Docker container. When a coordinator spawns multiple sub-tasks, each needs its own container, but they all currently share a single dockerContainerName. The authentication model for container-to-host networking (token passing via environment into an isolated container network) also needs a dedicated design pass. This is tracked as a known issue and is out of scope for this PR.


PS: I'm traveling this week, so if you find any issues I'll resolve them later in the week.

@johannesjo
Copy link
Copy Markdown
Owner

johannesjo commented May 11, 2026

Thank you very much for the update! <3

Reviewed the update range b55dcc9..c80a029 with multiple focused passes. I think these are worth addressing before merge:

  • P1: coordinator token scoping is bypassable. In electron/remote/server.ts:361-378 / 397-529, task-route access falls back to unscoped behavior when X-Coordinator-Id is missing or unregistered. Since all coordinator MCP configs use the same remoteServer.token (electron/ipc/register.ts:1394), a coordinator process that has that token can omit the header and list/control tasks owned by other coordinators. Coordinator-token task routes should require a valid coordinator identity, or use per-coordinator tokens.

  • P1: sub-task signal_done auth is not task-scoped. electron/remote/server.ts:192-193 allows the global subtaskToken to call any /api/tasks/:id/done, and electron/remote/server.ts:521-528 treats a missing coordinator header as unscoped. Any sub-task with access to its MCP config/token can signal completion for another task. This should be bound to the specific task id, ideally with per-task done tokens.

  • P1: atomic writes can fail across filesystems. electron/mcp/atomic.ts:14-17 and 34-37 create temp files in os.tmpdir() and then rename them to the target. That throws EXDEV when /tmp and the worktree/config path are on different mounts, which is common with tmpfs, external project volumes, or Docker/bind-mounted paths. This affects .mcp.json writes at electron/ipc/register.ts:1479 and Docker sub-task config writes at electron/mcp/coordinator.ts:596-597. The temp file needs to live in dirname(filePath).

  • P1: retry after MCP startup error does not spawn the terminal. src/components/TerminalView.tsx:641-653 only installs the ready-state watcher when the initial status is pending; if the component mounts in error, it skips both the watcher and spawnNow(). Clicking Retry at src/components/TerminalView.tsx:743 can move the store state to pending/ready, but the mount block does not re-run, so the overlay disappears and no PTY starts. Install the watcher regardless of initial status, guarded by a spawned flag.

  • P1: child hydration can mark children ready even when coordinator restore failed. src/App.tsx:374-400 uses Promise.allSettled and then hydrates every coordinated child regardless of the coordinator's MCP restore result. The IPC handler at electron/ipc/register.ts:1223-1241 also uses coordinator?.hydrateTask(...) and still emits MCP_TaskHydrated, so children can spawn with stale/broken MCP wiring when the coordinator is missing or in error. Gate hydration on coordinator ready, and make the IPC handler fail if the coordinator is unavailable/unregistered.

  • P2: custom Docker images are not propagated for new coordinator-created sub-tasks. Fresh coordinator creation and retry call IPC.StartMCPServer without dockerImage at src/store/tasks.ts:186-195 and 1176-1185. register.ts:1417 stores only args.dockerImage, and coordinator.ts:628 passes that to docker run, so MCP-created sub-tasks fall back to the default image. The restore path does pass it (src/App.tsx:363), but fresh creation/retry should too.

  • P2: enabling remote access after local MCP can return unreachable network URLs. If MCP starts first without Docker, StartMCPServer binds the shared server to 127.0.0.1 (electron/ipc/register.ts:1342-1343). Later StartRemoteServer just returns the existing server (electron/ipc/register.ts:1018-1027) while exposing WiFi/Tailscale URLs from electron/remote/server.ts:852+; those URLs cannot connect to a loopback-only listener. Manual remote start needs to rebind/restart or otherwise avoid advertising unreachable URLs.

  • P2: existing .mcp.json parallel-code entries are overwritten and deleted. The merge at electron/ipc/register.ts:1405-1408 preserves other server names, but overwrites any pre-existing mcpServers["parallel-code"]. Deregistration then deletes that key at electron/mcp/coordinator.ts:1258-1266. This can remove user-owned or concurrent config. Preserve/restore the previous value, or delete only when the current value still matches the entry this coordinator wrote.

Smaller follow-up: package.json:24-25 adds semgrep/gitleaks scripts, but those CLIs are not in devDependencies; npm ci alone will not make the new lint scripts reproducible unless they are installed another way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants