fix(orchestration): complete tasks on worker_done + coordinator UX fixes (#8030)

* fix(orchestration): complete worker tasks and improve coordinator UX * Fix orchestration lifecycle sender resolution and peek/check compat hand - Lifecycle sends (worker_done/heartbeat) now use ORCA_TERMINAL_HANDLE verbatim, skipping the liveness probe and pane remint that could block delivery during restarts or mismatch stale-runtime assignee handles. - --peek now round-trips as {peek:true, unread:false} so older runtimes that strip unknown params degrade to non-destructive "all" instead of mark-read, with client-side filtering to restore peek semantics and a clear error when --peek --wait can't be honored. - Reject combined read-mode flags (--unread/--peek/--all) before calling the runtime. - Distinguish suppressed (already-consumed) lifecycle messages from ignored ones so send doesn't wake --wait waiters for stale heartbeats. - Fix task summary truncation to avoid splitting UTF-16 surrogate pairs and to not misreport whitespace normalization as truncation. * Add shared helper to abbreviate orchestration task specs for brief listi - Normalizes whitespace and caps spec length at 160 chars, flagging truncation separately from whitespace-only changes - Truncates on UTF-16 code point boundaries to avoid splitting surrogate pairs and emitting malformed strings * Add pane-key identity to worker_done/heartbeat reconciliation and server - Records the sender's pane key on messages and dispatch contexts so worker_done/heartbeat ownership can be verified by the remint-stable pane leaf instead of the terminal handle, which is reissued across restarts. - Rejects lifecycle messages from a genuinely foreign pane while still tolerating handle remints, tab break-outs, and older CLIs that lack pane identity. - Moves task-spec abbreviation server-side (orchestration.taskList --brief) so full specs no longer cross SSH/relay transports, with a client-side fallback for older runtimes; consolidates the shared abbreviation helper under src/shared. - Adds a stderr warning when a pre-peek runtime's --peek response hits the 100-row cap, since older unread messages may be missing. * Isolate ORCA_PANE_KEY in CLI test beforeEach to fix leaked senderPaneKey Co-authored-by: Orca <help@stably.ai> * Fix pane-key remint bypassing dispatch mutual-exclusion lock - Dispatch locking only matched on assignee_handle, so a reminted terminal handle (tab break-out) could open a second concurrent dispatch on the same pane. - Add leaf-UUID-based pane key comparison (parsePaneKey) as a secondary lock, falling back to exact handle match for legacy rows without pane keys. * Update orchestration skill docs for lifecycle authority and CLI flag add - Clarify that dispatch lifecycle is tied to taskId+dispatchId verified against the dispatched pane, not the terminal handle, since handles can be reminted after restart - Document new `check --peek`/`--all` and `task-list --brief` flags, with fallback guidance for older CLIs that reject them - Note that a valid worker_done auto-completes the task/dispatch, so workers shouldn't also call task-update manually --------- Co-authored-by: Orca <help@stably.ai>
2026-10-27 16:02:42 +00:00 · 2026-07-12 02:15:11 -07:00
parent ee82d66a35
commit 26934b11bf
24 changed files with 1090 additions and 133 deletions
@@ -90,6 +90,59 @@ describe('orchestration RPC methods', () => {
      expect(runtime.deliverPendingMessagesForHandle).toHaveBeenCalledWith('term_b')
    })

+    it('stores the sender pane key on the message row', async () => {
+      setup()
+      vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {})
+      vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {})
+
+      const result = (await call('orchestration.send', {
+        from: 'term_a',
+        to: 'term_b',
+        subject: 'hello',
+        senderPaneKey: 'tab_a:leaf_a'
+      })) as { message: { id: string } }
+
+      expect(db.getMessageById(result.message.id)?.sender_pane_key).toBe('tab_a:leaf_a')
+    })
+
+    it('does not wake waiters for a heartbeat suppressed at send time', async () => {
+      setup()
+      const task = db.createTask({ spec: 'work' })
+      const dispatch = db.createDispatchContext(task.id, 'term_worker')
+      db.updateTaskStatus(task.id, 'completed')
+      vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {})
+      const notify = vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {})
+
+      const result = (await call('orchestration.send', {
+        from: 'term_worker',
+        to: 'term_coord',
+        subject: 'alive',
+        type: 'heartbeat',
+        payload: JSON.stringify({ dispatchId: dispatch.id })
+      })) as { message: { id: string } }
+
+      expect(notify).not.toHaveBeenCalled()
+      expect(db.getMessageById(result.message.id)).toMatchObject({ read: 1 })
+    })
+
+    it('still wakes waiters for a heartbeat on an active dispatch', async () => {
+      setup()
+      const task = db.createTask({ spec: 'work' })
+      const dispatch = db.createDispatchContext(task.id, 'term_worker')
+      vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {})
+      const notify = vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {})
+
+      await call('orchestration.send', {
+        from: 'term_worker',
+        to: 'term_coord',
+        subject: 'alive',
+        type: 'heartbeat',
+        payload: JSON.stringify({ dispatchId: dispatch.id })
+      })
+
+      expect(notify).toHaveBeenCalledWith('term_coord', 'heartbeat')
+    })
+
    it('rejects missing --to', () => {
      const method = findMethod('orchestration.send')
      expect(() => method.params!.parse({ subject: 'hi' })).toThrow()
@@ -598,11 +651,11 @@ describe('orchestration RPC methods', () => {
      expect(db.getDispatchContextById(dispatch.id)?.status).toBe('dispatched')
    })

-    it('does not complete worker_done from a terminal that does not own the dispatch', async () => {
+    it('completes worker_done by payload IDs when the sender handle changed', async () => {
      setup()
      const { task, dispatch } = createDispatchedTask('term_owner')
      insertWorkerDone({
-        from: 'term_intruder',
+        from: 'term_reminted',
        taskId: task.id,
        dispatchId: dispatch.id
      })
@@ -613,8 +666,8 @@ describe('orchestration RPC methods', () => {
      })) as { count: number }

      expect(result.count).toBe(1)
-      expect(db.getTask(task.id)?.status).toBe('dispatched')
-      expect(db.getDispatchContextById(dispatch.id)?.status).toBe('dispatched')
+      expect(db.getTask(task.id)?.status).toBe('completed')
+      expect(db.getDispatchContextById(dispatch.id)?.status).toBe('completed')
    })

    it('does not complete worker_done for a stale inactive dispatch', async () => {
@@ -670,6 +723,11 @@ describe('orchestration RPC methods', () => {
      ).rejects.toThrow('Invalid --types')
    })

+    it('rejects conflicting message read modes', () => {
+      const method = findMethod('orchestration.check')
+      expect(() => method.params!.parse({ unread: true, peek: true })).toThrow(/read mode/)
+    })
+
    it('default (unread only) marks returned rows as read', async () => {
      setup()
      db.insertMessage({ from: 'a', to: 'b', subject: 'one' })
@@ -686,6 +744,36 @@ describe('orchestration RPC methods', () => {
      expect(second.count).toBe(0)
    })

+    it('--peek returns unread messages without marking them read', async () => {
+      setup()
+      db.insertMessage({ from: 'a', to: 'b', subject: 'one' })
+
+      const result = (await call('orchestration.check', {
+        terminal: 'b',
+        peek: true
+      })) as { count: number }
+
+      expect(result.count).toBe(1)
+      expect(db.getUnreadMessages('b')).toHaveLength(1)
+    })
+
+    it("treats the CLI's {peek, unread:false} compat pair as peek, not all", async () => {
+      setup()
+      const seen = db.insertMessage({ from: 'a', to: 'b', subject: 'seen' })
+      db.markAsRead([seen.id])
+      db.insertMessage({ from: 'a', to: 'b', subject: 'fresh' })
+
+      const result = (await call('orchestration.check', {
+        terminal: 'b',
+        peek: true,
+        unread: false
+      })) as { messages: { subject: string }[]; count: number }
+
+      expect(result.count).toBe(1)
+      expect(result.messages[0]?.subject).toBe('fresh')
+      expect(db.getUnreadMessages('b')).toHaveLength(1)
+    })
+
    it('--all returns every message for the handle without marking read', async () => {
      setup()
      db.insertMessage({ from: 'a', to: 'b', subject: 'one' })
@@ -1000,6 +1088,24 @@ describe('orchestration RPC methods', () => {
    })
  })

+  describe('orchestration.taskList --brief', () => {
+    it('abbreviates specs server-side so full text never crosses the wire', async () => {
+      setup()
+      db.createTask({ spec: `First line\n${'detail '.repeat(40)}` })
+      db.createTask({ spec: 'Short task' })
+
+      const result = (await call('orchestration.taskList', { brief: true })) as {
+        tasks: { spec: string; spec_truncated: boolean }[]
+      }
+
+      const [long, short] = result.tasks
+      expect(long.spec).toHaveLength(160)
+      expect(long.spec_truncated).toBe(true)
+      expect(short.spec).toBe('Short task')
+      expect(short.spec_truncated).toBe(false)
+    })
+  })
+
  describe('orchestration.taskUpdate', () => {
    it('updates task status', async () => {
      setup()
@@ -1050,6 +1156,20 @@ describe('orchestration RPC methods', () => {
      expect(result.dispatch.status).toBe('dispatched')
    })

+    it('records the assignee pane key on the dispatch context', async () => {
+      setup()
+      vi.spyOn(runtime, 'getTerminalPaneKey').mockReturnValue('tab_w:leaf_w')
+      const task = db.createTask({ spec: 'work' })
+
+      const result = (await call('orchestration.dispatch', {
+        task: task.id,
+        to: 'term_a'
+      })) as { dispatch: { id: string } }
+
+      expect(runtime.getTerminalPaneKey).toHaveBeenCalledWith('term_a')
+      expect(db.getDispatchContextById(result.dispatch.id)?.assignee_pane_key).toBe('tab_w:leaf_w')
+    })
+
    it('rejects dispatch for a pending task', async () => {
      setup()
      const parent = db.createTask({ spec: 'parent' })
@@ -7,6 +7,7 @@ import { buildDispatchPreamble } from '../../orchestration/preamble'
 import { formatMessageBanner } from '../../orchestration/formatter'
 import { isGroupAddress, resolveGroupAddress } from '../../orchestration/groups'
 import { reconcileLifecycleMessage } from '../../orchestration/lifecycle-reconciliation'
+import { abbreviateOrchestrationTasks } from '../../../../shared/orchestration-task-summary'
 import { ORCHESTRATION_GATE_METHODS } from './orchestration-gates'

 const MESSAGE_TYPES: MessageType[] = [
@@ -54,6 +55,9 @@ const SendParams = z
    priority: z.enum(['normal', 'high', 'urgent']).optional(),
    threadId: OptionalString,
    payload: OptionalString,
+    // Why: the sender's pane key is the remint-stable identity used to verify
+    // worker_done/heartbeat ownership; the from handle stays routing metadata.
+    senderPaneKey: OptionalString,
    devMode: OptionalBoolean
  })
  .superRefine((params, ctx) => {
@@ -72,18 +76,36 @@ const SendParams = z
    })
  })

-const CheckParams = z.object({
-  terminal: OptionalString,
-  unread: OptionalBoolean,
-  // Why: `all` surfaces every message for the handle and skips mark-read.
-  // Previously the only way to ask for "all" was the hidden RPC trick
-  // `{unread: false}`. See design doc §3.2 / §3.3.
-  all: OptionalBoolean,
-  types: OptionalString,
-  inject: OptionalBoolean,
-  wait: OptionalBoolean,
-  timeoutMs: OptionalFiniteNumber
-})
+const CheckParams = z
+  .object({
+    terminal: OptionalString,
+    unread: OptionalBoolean,
+    peek: OptionalBoolean,
+    // Why: `all` surfaces every message for the handle and skips mark-read.
+    // Previously the only way to ask for "all" was the hidden RPC trick
+    // `{unread: false}`. See design doc §3.2 / §3.3.
+    all: OptionalBoolean,
+    types: OptionalString,
+    inject: OptionalBoolean,
+    wait: OptionalBoolean,
+    timeoutMs: OptionalFiniteNumber
+  })
+  .superRefine((params, ctx) => {
+    // Why: the CLI encodes --peek as {peek:true, unread:false} so pre-peek
+    // runtimes degrade to the non-consuming all mode; that pair is one mode,
+    // not a conflict.
+    const modes = [
+      params.unread === true,
+      params.peek === true,
+      params.all === true || (params.unread === false && params.peek !== true)
+    ].filter(Boolean)
+    if (modes.length > 1) {
+      ctx.addIssue({
+        code: z.ZodIssueCode.custom,
+        message: 'Choose at most one message read mode: --unread, --peek, or --all.'
+      })
+    }
+  })

 const ReplyParams = z.object({
  id: requiredString('Missing --id'),
@@ -110,7 +132,10 @@ const TaskCreateParams = z.object({

 const TaskListParams = z.object({
  status: z.enum(['pending', 'ready', 'dispatched', 'completed', 'failed', 'blocked']).optional(),
-  ready: OptionalBoolean
+  ready: OptionalBoolean,
+  // Why: truncating specs server-side keeps `--brief` cheap over SSH/relay
+  // transports instead of shipping full specs the CLI then throws away.
+  brief: OptionalBoolean
 })

 const TaskUpdateParams = z.object({
@@ -195,14 +220,20 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [
          type: params.type as MessageType,
          priority: params.priority as MessagePriority,
          threadId: params.threadId,
-          payload: params.payload
+          payload: params.payload,
+          senderPaneKey: params.senderPaneKey
        })
        // Why: worker_done/heartbeat sent via `send` must release the dispatch
        // lock before waking recipients — a coordinator woken by delivery may
        // immediately dispatch to the same terminal, which fails if the lock
        // is still held.
        if (msg.type === 'worker_done' || msg.type === 'heartbeat') {
-          reconcileLifecycleMessage(db, msg)
+          const reconciled = reconcileLifecycleMessage(db, msg)
+          // Why: a suppressed message is already read; waking a `check --wait`
+          // waiter for it would return an empty result before the deadline.
+          if (reconciled.action === 'suppressed') {
+            return { message: msg }
+          }
        }
        runtime.deliverPendingMessagesForHandle(params.to)
        runtime.notifyMessageArrived(params.to, msg.type)
@@ -231,7 +262,8 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [
          type: params.type as MessageType,
          priority: params.priority as MessagePriority,
          threadId,
-          payload: params.payload
+          payload: params.payload,
+          senderPaneKey: params.senderPaneKey
        })
      )
      for (const message of messages) {
@@ -264,15 +296,15 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [
      // Explicit `unread: false` is also honored for one release as a compat
      // shim so in-flight callers don't break (see design doc §5). Otherwise
      // today's behavior is preserved: default is unread-only + mark-read.
-      const showAll = params.all === true || params.unread === false
-      const showUnread = !showAll
+      const showAll = params.all === true || (params.unread === false && params.peek !== true)
+      const consumeUnread = !showAll && params.peek !== true

      const readAndReturn = () => {
-        const messages = showUnread
-          ? db.getUnreadMessages(handle, typeFilter)
-          : db.getAllMessagesForHandle(handle, undefined, typeFilter)
+        const messages = showAll
+          ? db.getAllMessagesForHandle(handle, undefined, typeFilter)
+          : db.getUnreadMessages(handle, typeFilter)

-        if (showUnread && messages.length > 0) {
+        if (consumeUnread && messages.length > 0) {
          // Why: manual coordinators can consume lifecycle messages before
          // the coordinator loop sees them, but unread `check` is still an
          // authoritative read path for worker_done/heartbeat.
@@ -406,7 +438,10 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [
        }
        return base
      })
-      return { tasks, count: tasks.length }
+      return {
+        tasks: params.brief ? abbreviateOrchestrationTasks(tasks) : tasks,
+        count: tasks.length
+      }
    }
  }),

@@ -475,7 +510,11 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [
        }
      }

-      const ctx = db.createDispatchContext(params.task, to)
+      const ctx = db.createDispatchContext(
+        params.task,
+        to,
+        runtime.getTerminalPaneKey(to) ?? undefined
+      )

      // Why: preamble is built here (not before ctx) so `dispatchId` can be
      // the real ctx.id — the preamble-hardening PR made dispatchId required
@@ -472,14 +472,18 @@ describe('worktree RPC methods', () => {
    const runtime = {
      getRuntimeId: () => 'test-runtime',
      showRepo: vi.fn().mockResolvedValue(repo),
-      createManagedWorktree: vi.fn().mockResolvedValue({ worktree: { id: 'wt-1' } })
+      createManagedWorktree: vi.fn().mockResolvedValue({
+        worktree: { id: 'wt-1' },
+        startupTerminal: { spawned: true, handle: 'term_agent' }
+      })
    } as unknown as OrcaRuntimeService
    const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS })

-    await dispatcher.dispatch(
+    const response = await dispatcher.dispatch(
      makeRequest('worktree.create', {
        repo: 'repo-1',
        name: 'agent-startup',
+        startupAgent: 'codex',
        startupCommand: "codex 'summarize repo'",
        startupCommandDelivery: 'shell-ready',
        startupEnv: { ORCA_AGENT_MODE: 'direct' },
@@ -492,11 +496,17 @@ describe('worktree RPC methods', () => {
      })
    )

+    expect(response).toMatchObject({
+      ok: true,
+      result: { agentTerminalHandle: 'term_agent' }
+    })
+
    expect(runtime.createManagedWorktree).toHaveBeenCalledWith(
      expect.objectContaining({
        repoSelector: 'repo-1',
        name: 'agent-startup',
        activate: true,
+        startupAgent: 'codex',
        startup: {
          command: "codex 'summarize repo'",
          startupCommandDelivery: 'shell-ready',
@@ -133,7 +133,11 @@ export const WORKTREE_METHODS: RpcMethod[] = [
          }
        })
        finishAutomationWorkspaceProvenanceRequest(params.automationProvenanceRequest)
-        return result
+        // Why: agent callers need a stable dispatch target without traversing
+        // terminal-list layout duplicates after creating the worktree.
+        return params.startupAgent && result.startupTerminal?.handle
+          ? { ...result, agentTerminalHandle: result.startupTerminal.handle }
+          : result
      } catch (error) {
        releaseAutomationWorkspaceProvenanceRequest(params.automationProvenanceRequest)
        throw error