refactor(coordinator): address PR johannesjo#145 review feedback

brooksc · claude · johannesjo · commit a9e28f8d9b32 · 2026-05-27T12:20:50.000+02:00
- Move verification/summary write after validation in landSelf so rejected
  calls don't persist bogus data to the renderer
- Comment the landing_escalated escape hatch in assertTaskCanBeMerged
- Replace inline isLandedTaskState closures in TaskPanel, TaskTitleBar,
  TerminalView with the shared predicate from store/landing
- Extract GIT_LOCK_RETRY_DELAY_MS constant for the index.lock retry sleep
- Add comment explaining PROMPT_ECHO_HANDOFF_SUPPRESS_MS heuristic
- Early-return in applyTaskMcpLaunchResult when task is missing (dead path)
- Assert only preamble paths are staged before cleanup commit
- Extract detachMcpState variable in syncLandingState for clarity
- Remove wait_for_signal_done from coordinator preamble (noise; signalDoneAt
  is already visible in list_tasks/get_task_status)

Co-Authored-By: Claude Sonnet 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/electron/mcp/coordinator.ts b/electron/mcp/coordinator.ts
@@ -66,6 +66,7 @@ import { IPC } from '../ipc/channels.js';
 
 const DEFAULT_WAIT_TIMEOUT_MS = 300_000; // 5 minutes
 const PROMPT_WRITE_DELAY_MS = 50;
+const GIT_LOCK_RETRY_DELAY_MS = 2_000;
 const REST_COORDINATOR_SENTINEL = 'api';
 const PREAMBLE_ARTIFACT_PATHS = new Set([
   'AGENTS.md',
@@ -1000,6 +1001,11 @@ export class Coordinator {
   }
 
   private syncLandingState(task: CoordinatedTask): void {
+    // Detach renderer MCP state only after a successful merge (landed states);
+    // escalation/failure states leave it attached so the task stays reachable.
+    const detachMcpState =
+      task.landingState === 'landed_pending_review' ||
+      task.landingState === 'landed_cleanup_failed';
     this.notifyRenderer(IPC.MCP_TaskStateSync, {
       taskId: task.id,
       verification: task.verification,
@@ -1012,21 +1018,9 @@ export class Coordinator {
         task.landingState === 'landed_cleanup_failed' ||
         task.landingState === 'landing_escalated' ||
         task.landingState === 'landing_failed',
-      controlledBy:
-        task.landingState === 'landed_pending_review' ||
-        task.landingState === 'landed_cleanup_failed'
-          ? null
-          : undefined,
-      mcpConfigPath:
-        task.landingState === 'landed_pending_review' ||
-        task.landingState === 'landed_cleanup_failed'
-          ? null
-          : undefined,
-      mcpStartupStatus:
-        task.landingState === 'landed_pending_review' ||
-        task.landingState === 'landed_cleanup_failed'
-          ? null
-          : undefined,
+      controlledBy: detachMcpState ? null : undefined,
+      mcpConfigPath: detachMcpState ? null : undefined,
+      mcpStartupStatus: detachMcpState ? null : undefined,
     });
   }
 
@@ -1082,6 +1076,15 @@ export class Coordinator {
     }
 
     if (dirtyPaths.length > 0) {
+      // nonPreamblePaths check above guarantees every path here is a preamble artifact.
+      const unexpectedPaths = dirtyPaths.filter(
+        (p) => !preambleFiles.has(p) || !PREAMBLE_ARTIFACT_PATHS.has(p),
+      );
+      if (unexpectedPaths.length > 0) {
+        throw new Error(
+          `Unexpected non-preamble paths staged before cleanup commit: ${unexpectedPaths.join(', ')}`,
+        );
+      }
       await execAsync('git', ['add', '-A', '--', ...dirtyPaths], { cwd: task.worktreePath });
       try {
         await execAsync('git', ['commit', '-m', 'Remove Parallel Code sub-task preamble'], {
@@ -1122,7 +1125,7 @@ export class Coordinator {
     } catch (err) {
       const msg = err instanceof Error ? err.message : String(err);
       if (msg.includes('Another git process') || msg.includes('index.lock')) {
-        await new Promise((r) => setTimeout(r, 2000));
+        await new Promise((r) => setTimeout(r, GIT_LOCK_RETRY_DELAY_MS));
         result = await runMerge();
       } else {
         throw err;
@@ -1210,9 +1213,6 @@ export class Coordinator {
     const task = this.tasks.get(taskId);
     if (!task) throw new Error(`Task not found: ${taskId}`);
 
-    task.verification = input.verification;
-    task.landingSummary = input.summary;
-
     if (!this.coordinators.has(task.coordinatorTaskId)) {
       const reason = `Cannot self-land orphaned task: coordinator ${task.coordinatorTaskId} is not registered`;
       this.escalateLanding(task, 'landing_escalated', reason);
@@ -1225,6 +1225,11 @@ export class Coordinator {
       throw new Error(reason);
     }
 
+    // Only persist verification/summary after all validation passes so rejected
+    // calls don't leave bogus data visible in the renderer.
+    task.verification = input.verification;
+    task.landingSummary = input.summary;
+
     try {
       await this.prepareCleanSelfLandingWorktree(task);
     } catch (err) {
@@ -1366,6 +1371,10 @@ export class Coordinator {
     }
 
     const hasManualReviewSignal = task.signalDoneAt !== undefined;
+    // landing_escalated unlocks merge_task as an escape hatch for the coordinator.
+    // This is not an authority leak: sub-task tokens can only reach land_self (via
+    // the /api/tasks/:id/land endpoint); merge_task is a coordinator-only MCP tool,
+    // so only a coordinator agent can invoke it here.
     const hasLandingEscalation =
       task.landingState === 'landing_escalated' || task.landingState === 'landing_failed';
     if (task.status === 'running' && !hasManualReviewSignal && !hasLandingEscalation) {
diff --git a/src/components/PromptInput.tsx b/src/components/PromptInput.tsx
@@ -81,6 +81,8 @@ const AUTOSEND_MAX_WAIT_MS = 45_000;
 const PROMPT_VERIFY_TIMEOUT_MS = 5_000;
 const PROMPT_VERIFY_POLL_MS = 250;
 const PROMPT_MARKER_SCAN_CHARS = 500;
+// 5s covers the typical round-trip from send → pty echo; 40 chars is enough to
+// uniquely identify the prompt without risking a false match on short snippets.
 const PROMPT_ECHO_HANDOFF_SUPPRESS_MS = 5_000;
 
 /** True when auto-send should be blocked by a question in the output.
diff --git a/src/components/TaskPanel.tsx b/src/components/TaskPanel.tsx
@@ -42,6 +42,7 @@ import { theme } from '../lib/theme';
 import { isMac } from '../lib/platform';
 import type { Task } from '../store/types';
 import type { CommitInfo } from '../ipc/types';
+import { isLandedTaskState } from '../store/landing';
 
 interface TaskPanelProps {
   task: Task;
@@ -78,10 +79,7 @@ export function TaskPanel(props: TaskPanelProps) {
   const [showPushConfirm, setShowPushConfirm] = createSignal(false);
   const [pushSuccess, setPushSuccess] = createSignal(false);
   const [pushing, setPushing] = createSignal(false);
-  const isLandedTask = () =>
-    props.task.landingState === 'landed_pending_review' ||
-    props.task.landingState === 'landed_cleanup_failed' ||
-    props.task.landingState === 'reviewed';
+  const isLandedTask = () => isLandedTaskState(props.task.landingState);
   let pushSuccessTimer: ReturnType<typeof setTimeout> | undefined;
   onCleanup(() => clearTimeout(pushSuccessTimer));
   const [diffScrollTarget, setDiffScrollTarget] = createSignal<string | null>(null);
diff --git a/src/components/TaskTitleBar.tsx b/src/components/TaskTitleBar.tsx
@@ -19,6 +19,7 @@ import { handleDragReorder } from '../lib/dragReorder';
 import { getTaskDockerBadgeLabel } from '../lib/docker';
 import { displayTaskNameFromPrompt, shouldUsePromptDerivedTaskName } from '../lib/clean-task-name';
 import type { Task } from '../store/types';
+import { isLandedTaskState } from '../store/landing';
 
 interface TaskTitleBarProps {
   task: Task;
@@ -33,10 +34,7 @@ interface TaskTitleBarProps {
 
 export function TaskTitleBar(props: TaskTitleBarProps) {
   const dockerBadgeLabel = () => getTaskDockerBadgeLabel(props.task.dockerSource);
-  const isLandedTask = () =>
-    props.task.landingState === 'landed_pending_review' ||
-    props.task.landingState === 'landed_cleanup_failed' ||
-    props.task.landingState === 'reviewed';
+  const isLandedTask = () => isLandedTaskState(props.task.landingState);
   const landingBadge = () => {
     switch (props.task.landingState) {
       case 'landed_pending_review':
diff --git a/src/components/TerminalView.tsx b/src/components/TerminalView.tsx
@@ -657,12 +657,7 @@ export function TerminalView(props: TerminalViewProps) {
     function startSpawn() {
       if (!term || spawnStarted) return;
       const landingState = store.tasks[taskId]?.landingState;
-      if (
-        landingState === 'landed_pending_review' ||
-        landingState === 'landed_cleanup_failed' ||
-        landingState === 'reviewed'
-      )
-        return;
+      if (isLandedTaskState(landingState)) return;
       spawnStarted = true;
       invoke(IPC.SpawnAgent, {
         taskId,
diff --git a/src/store/coordinator-preamble.ts b/src/store/coordinator-preamble.ts
@@ -10,7 +10,6 @@ You have MCP tools to coordinate work across isolated git worktree tasks:
 - list_tasks — List all coordinated tasks with status
 - get_task_status — Detailed status of a task
 - send_prompt — Send follow-up instructions to a task's agent
-- wait_for_signal_done — Legacy/manual-review wait for ANY sub-task to call signal_done. Normal self-landing tasks do not call it.
 - wait_for_idle — Wait until an agent is idle at its prompt (use for send_prompt follow-ups)
 - get_task_diff — Get changed files and diff for a task
 - get_task_output — Get recent terminal output from a task
diff --git a/src/store/tasks.ts b/src/store/tasks.ts
@@ -1328,6 +1328,7 @@ export function applyTaskMcpLaunchResult(
   taskId: string,
   result: { mcpLaunchArgs?: string[] } | undefined,
 ): boolean {
+  if (!store.tasks[taskId]) return false;
   const args = result?.mcpLaunchArgs;
   if ((!Array.isArray(args) || args.length === 0) && taskRequiresMcpLaunchArgs(taskId)) {
     markTaskMcpError(taskId, 'MCP startup returned no launch args');
