feat(orchestration): stop confirming closes of an agent's own children

src/renderer/src/workspace/hook/actions/pane.ts

@@ -106,9 +106,41 @@ function forgetClosedSessionDebugState(refs: WorkspaceRefs, sessionId: SessionId
  * — omit it — is the gated path, and the Agent Activity modal's Close button is
  * exactly the case that must keep paying for it: a single button in a list, no
  * preview of the cascade it triggers.
+ *
+ * A fourth caller does NOT qualify and must not assert it: orchestration MCP.
+ * See `silentIfSoleTarget` below for what it uses instead and why.
  */
 export type CloseSessionOptions = {
   preConfirmed?: boolean
+  /**
+   * Skip the dialog ONLY IF this close kills exactly the session named, and
+   * confirm with `headline` otherwise.
+   *
+   * WHY orchestration needs its own mode rather than `preConfirmed`: an
+   * orchestrating agent closing children it created is routine housekeeping,
+   * and a dialog per close devalues the confirmations that matter. But the
+   * ownership gate that authorizes the call only scopes WHICH SESSION MAY BE
+   * NAMED — it says nothing about WHICH SESSIONS DIE. `closeSession` kills a
+   * SET, and two real shapes reach beyond the named target:
+   *
+   *   - `closeLinkedChildren` takes every session linked to the target. A user
+   *     can run "Linked Agent…" on an orchestration child, and that linked
+   *     agent carries no orchestration fields at all — so the gate cannot see
+   *     it, and asserting preConfirmed would kill a session the user built by
+   *     hand, silently.
+   *   - Closing a tab's SOLE grid leaf takes every detached session in that
+   *     tab. Orchestration children live detached in the root's tab, so that
+   *     set can include the caller itself, its siblings, and Dispatch agents
+   *     the user parked there.
+   *
+   * Agent Management refuses both shapes outright (`additionalCloseImpact`)
+   * even though it has a dialog available. Orchestration cannot refuse — a
+   * fleet must be able to clean up — so it confirms instead, but only for the
+   * shapes that actually reach further than advertised. The routine case (a
+   * detached child with no linked children) expands to exactly one and stays
+   * silent, which is the entire point.
+   */
+  silentIfSoleTarget?: { headline: string }
   /**
    * Confirm even when the policy would let this through silently.
    *
@@ -1447,12 +1479,32 @@ export function usePaneActions(
       // straight through, cascade and all, with no dialog. `closeFocused`
       // delegates here for its Dispatch and detached-child arms and passes
       // preConfirmed, so those still ask exactly once.
-      if (!options?.preConfirmed) {
+      // Resolve the orchestration mode into the two existing ones, HERE, where
+      // paneCloseTargets is in scope — it is the only code that computes the
+      // full set a close destroys, which is exactly what the caller cannot
+      // know from the outside.
+      let effectivePreConfirmed = options?.preConfirmed === true
+      let effectiveRequireConfirmation = options?.requireConfirmation
+      if (options?.silentIfSoleTarget) {
+        const expanded = paneCloseTargets(
+          refs.stateRef.current,
+          refs.latestRuntimesRef.current,
+          targetId,
+        )
+        const soleTarget = expanded.length === 1 && expanded[0]?.sessionId === targetId
+        if (soleTarget) {
+          effectivePreConfirmed = true
+        } else {
+          effectiveRequireConfirmation = options.silentIfSoleTarget
+        }
+      }
+
+      if (!effectivePreConfirmed) {
         const gate = await runCloseConfirmationGate({
           enumerate: () =>
             paneCloseTargets(refs.stateRef.current, refs.latestRuntimesRef.current, targetId),
           ask: requestCloseConfirmation,
-          force: options?.requireConfirmation,
+          force: effectiveRequireConfirmation,
         })
         if (!gate.ok) {
           if (gate.reason === 'changed') showToast(CLOSE_CHANGED_TOAST)

src/renderer/src/workspace/hook/index.ts

@@ -412,25 +412,7 @@ export function useWorkspace(
             state: snapshot,
             parentSessionId: request.parentSessionId,
             sessionId: request.sessionId,
-            // Adapted to the orchestration contract's Promise<void>.
-            //
-            // NOT because the signal is useless — it is not. `OrchestrationClose-
-            // Result` carries `skippedSessionIds` for exactly this, and
-            // closeOrchestrationRun's own comment claims "declining it throws,
-            // which the catch below turns into a skip" — which is false:
-            // requestCloseConfirmation RESOLVES false, it never rejects. So an
-            // orchestrating agent that has its close declined by the user is
-            // currently told the child closed. That is a real pre-existing bug
-            // and this boolean is the missing half of its fix.
-            //
-            // It is deliberately NOT fixed here: changing what an orchestrating
-            // agent is told about a close is a cross-process behaviour change
-            // that deserves its own review, not a ride-along in a
-            // Dispatch-lane PR. Filed as follow-up work; do not "tidy" this
-            // comment away without doing it.
-            closeSession: async (id, opts) => {
-              await closeOrchestrationSessionRef.current(id, opts)
-            },
+            closeSession: closeOrchestrationSessionRef.current,
           })
           await window.api.resolveOrchestrationRequest({
             requestId: request.requestId,
@@ -505,10 +487,7 @@ export function useWorkspace(
           state: snapshot,
           parentSessionId: request.parentSessionId,
           runId: request.runId,
-          // Same adaptation as the single-agent close above.
-          closeSession: async (id, opts) => {
-            await closeOrchestrationSessionRef.current(id, opts)
-          },
+          closeSession: closeOrchestrationSessionRef.current,
         })
         await window.api.resolveOrchestrationRequest({
           requestId: request.requestId,
@@ -780,13 +759,24 @@ export function useWorkspace(
           // idle target does not slip through the human-ergonomics exemption
           // the policy grants ⌘W. Declining rejects the tool call.
           const caller = current.sessions[request.callerSessionId]?.title
-          await closeOrchestrationSessionRef.current(request.sessionId, {
+          const closed = await closeOrchestrationSessionRef.current(request.sessionId, {
             requireConfirmation: {
               headline: caller
                 ? `Agent “${caller}” is asking to close this agent.`
                 : 'An agent is asking to close this agent.',
             },
           })
+          // The comment above says "declining rejects the tool call" — it did
+          // not. The gate RESOLVES false rather than throwing, so the success
+          // below fired anyway and the calling agent was told the close took
+          // while its target kept running. This is the surface where a decline
+          // is still possible, so it is the surface where reporting it matters
+          // most.
+          if (!closed) {
+            throw new Error(
+              'close_agent was declined: the user did not approve closing this agent.',
+            )
+          }
         }
         await window.api.resolveAgentManagementRequest({
           requestId: request.requestId,

src/renderer/src/workspace/orchestrationClose.test.ts

@@ -0,0 +1,252 @@
+import { describe, expect, it, vi } from 'vitest'
+
+import {
+  closeOrchestrationAgent,
+  closeOrchestrationRun,
+} from '@renderer/workspace/orchestrationMcp'
+import type { SessionId, SessionMeta, WorkspaceState } from '@renderer/workspace/types'
+
+// These two functions decide what an orchestrating agent is TOLD about a close,
+// and that report is acted on — a parent that believes a child closed will stop
+// waiting on it. The reporting was previously unconditional: every id went into
+// `closedSessionIds` whether or not the close took. So the contract worth
+// pinning is not "does it call closeSession" but "does it tell the truth".
+//
+// The unconditional confirmation these calls used to raise is gone, replaced by
+// `silentIfSoleTarget` (see the WHY on OrchestrationCloseSession). That option
+// is the whole safety argument: it stays silent when the close kills exactly
+// the named agent, and asks when it would reach further — a linked agent the
+// user attached, or a tab's sole leaf taking every detached session with it.
+//
+// So the option itself is part of the contract in both directions: a regression
+// that swapped it for `preConfirmed: true` would destroy user-created sessions
+// silently, and one that dropped it entirely would put a dialog back in front of
+// routine fleet cleanup.
+
+const PARENT = 'parent-1' as SessionId
+
+const child = (overrides: Partial<SessionMeta> = {}): SessionMeta => ({
+  cwd: '/tmp/project',
+  kind: 'claude',
+  orchestrationParentId: PARENT,
+  orchestrationRootId: PARENT,
+  ...overrides,
+}) as SessionMeta
+
+/**
+ * Narrow harness: both functions under test read ONLY `state.sessions`, so a
+ * bare sessions map is the whole input surface.
+ *
+ * WHY the `unknown` cast (which docs/testing/standard.md otherwise forbids):
+ * `WorkspaceState` carries a dozen fields — tabs, detachedSessions, dispatch
+ * mode, tile tree — that neither function touches, and building them would be
+ * a fixture that lies about what is being exercised.
+ *
+ * WHAT WOULD REMOVE IT: if either function starts reading `state.tabs` or
+ * `state.detachedSessions`, this cast will keep compiling while the fixture
+ * silently supplies `undefined`. That is exactly where `closeSession`'s own
+ * true/false answer comes from, so if these functions ever reach for it, build
+ * a real state factory instead of widening this one.
+ */
+const stateWith = (sessions: Record<string, SessionMeta>): WorkspaceState =>
+  ({ sessions } as unknown as WorkspaceState)
+
+describe('closeOrchestrationAgent', () => {
+  it('closes without raising a confirmation', async () => {
+    const closeSession = vi.fn().mockResolvedValue(true)
+    await closeOrchestrationAgent({
+      state: stateWith({ 'child-1': child() }),
+      parentSessionId: PARENT,
+      sessionId: 'child-1',
+      closeSession,
+    })
+    // NOT preConfirmed: the ownership gate scopes which session may be NAMED,
+    // not which sessions die, so the decision has to be made where the full
+    // expanded target set is computable.
+    expect(closeSession).toHaveBeenCalledWith('child-1', {
+      silentIfSoleTarget: { headline: expect.stringContaining('asking to close') },
+    })
+  })
+
+  it('reports the id as closed when the close took', async () => {
+    const result = await closeOrchestrationAgent({
+      state: stateWith({ 'child-1': child() }),
+      parentSessionId: PARENT,
+      sessionId: 'child-1',
+      closeSession: vi.fn().mockResolvedValue(true),
+    })
+    expect(result).toEqual({ closedSessionIds: ['child-1'] })
+  })
+
+  it('reports the id as SKIPPED when the close did not take', async () => {
+    // The regression this exists for: the old code returned the id in
+    // closedSessionIds unconditionally, so an already-gone agent was reported
+    // as closed and the parent proceeded on that.
+    const result = await closeOrchestrationAgent({
+      state: stateWith({ 'child-1': child() }),
+      parentSessionId: PARENT,
+      sessionId: 'child-1',
+      closeSession: vi.fn().mockResolvedValue(false),
+    })
+    expect(result).toEqual({ closedSessionIds: [], skippedSessionIds: ['child-1'] })
+  })
+
+  it('reports a thrown close as skipped rather than failing the tool call', async () => {
+    // close_agent had no catch: killSessionBackendIfOwned is IPC and can
+    // reject, and it runs AFTER closeLinkedChildren — so a bare throw could
+    // surface as a tool error while children were already gone, reported as
+    // neither closed nor skipped.
+    const result = await closeOrchestrationAgent({
+      state: stateWith({ 'child-1': child() }),
+      parentSessionId: PARENT,
+      sessionId: 'child-1',
+      closeSession: vi.fn().mockRejectedValue(new Error('backend kill failed')),
+    })
+    expect(result).toEqual({ closedSessionIds: [], skippedSessionIds: ['child-1'] })
+  })
+
+  it('refuses a session the caller does not own', async () => {
+    // The ownership gate is the safety property that removing the dialog rests
+    // on, so it is pinned here rather than assumed.
+    const closeSession = vi.fn().mockResolvedValue(true)
+    await expect(
+      closeOrchestrationAgent({
+        state: stateWith({
+          stranger: child({ orchestrationParentId: 'someone-else' as SessionId, orchestrationRootId: 'someone-else' as SessionId }),
+        }),
+        parentSessionId: PARENT,
+        sessionId: 'stranger',
+        closeSession,
+      }),
+    ).rejects.toThrow(/not found/i)
+    expect(closeSession).not.toHaveBeenCalled()
+  })
+})
+
+describe('closeOrchestrationRun', () => {
+  it('closes every child without a confirmation on any of them', async () => {
+    const closeSession = vi.fn().mockResolvedValue(true)
+    const result = await closeOrchestrationRun({
+      state: stateWith({ 'c1': child(), 'c2': child(), 'c3': child() }),
+      parentSessionId: PARENT,
+      closeSession,
+    })
+    expect(result.closedSessionIds.sort()).toEqual(['c1', 'c2', 'c3'])
+    // Previously the FIRST call carried requireConfirmation and the rest rode
+    // that answer, which meant one decline silently mis-reported the rest.
+    // Now every call carries the same sole-target mode and is judged on its own
+    // blast radius.
+    for (const call of closeSession.mock.calls) {
+      expect(call[1]).toEqual({
+        silentIfSoleTarget: { headline: expect.stringContaining('asking to close') },
+      })
+    }
+  })
+
+  it('separates the ones that did not close', async () => {
+    const closeSession = vi.fn(async (id: string) => id !== 'c2')
+    const result = await closeOrchestrationRun({
+      state: stateWith({ 'c1': child(), 'c2': child(), 'c3': child() }),
+      parentSessionId: PARENT,
+      closeSession,
+    })
+    expect(result.closedSessionIds.sort()).toEqual(['c1', 'c3'])
+    expect(result.skippedSessionIds).toEqual(['c2'])
+  })
+
+  it('names the requesting agent in the fallback headline', async () => {
+    // The headline is only ever shown when the close reaches past the named
+    // agent — a dialog the user did not summon. It has to say who asked.
+    const closeSession = vi.fn().mockResolvedValue(true)
+    await closeOrchestrationRun({
+      state: stateWith({
+        [PARENT]: { cwd: '/tmp/p', kind: 'claude', title: 'Reviewer' } as SessionMeta,
+        c1: child(),
+      }),
+      parentSessionId: PARENT,
+      closeSession,
+    })
+    expect(closeSession.mock.calls[0][1].silentIfSoleTarget.headline).toContain('Reviewer')
+  })
+
+  it('treats a thrown close as a skip and keeps going', async () => {
+    // One agent whose backend kill fails must not abort the rest of the run.
+    const closeSession = vi.fn(async (id: string) => {
+      if (id === 'c1') throw new Error('backend kill failed')
+      return true
+    })
+    const result = await closeOrchestrationRun({
+      state: stateWith({ 'c1': child(), 'c2': child() }),
+      parentSessionId: PARENT,
+      closeSession,
+    })
+    expect(result.skippedSessionIds).toEqual(['c1'])
+    expect(result.closedSessionIds).toEqual(['c2'])
+  })
+
+  it('omits skippedSessionIds entirely when nothing was skipped', async () => {
+    // The field is optional in the result contract; emitting an empty array
+    // would make a caller that checks presence think something went wrong.
+    const result = await closeOrchestrationRun({
+      state: stateWith({ 'c1': child() }),
+      parentSessionId: PARENT,
+      closeSession: vi.fn().mockResolvedValue(true),
+    })
+    // `in`, not toBeUndefined: the latter also passes for an explicitly-present
+    // `undefined`, which is not what "omits entirely" claims.
+    expect('skippedSessionIds' in result).toBe(false)
+  })
+
+  it('closes only the named run when a runId is given', async () => {
+    // With the unconditional dialog gone, this filter is the only thing
+    // separating "close this run" from "close every child I have ever
+    // started". It had no coverage at all before.
+    const closeSession = vi.fn().mockResolvedValue(true)
+    await closeOrchestrationRun({
+      state: stateWith({
+        a1: child({ orchestrationRunId: 'run-a' } as Partial<SessionMeta>),
+        a2: child({ orchestrationRunId: 'run-a' } as Partial<SessionMeta>),
+        b1: child({ orchestrationRunId: 'run-b' } as Partial<SessionMeta>),
+      }),
+      parentSessionId: PARENT,
+      runId: 'run-a',
+      closeSession,
+    })
+    expect(closeSession.mock.calls.map(c => c[0]).sort()).toEqual(['a1', 'a2'])
+  })
+
+  it('excludes non-agent sessions such as terminals', async () => {
+    // A terminal parked in the same run is not an orchestration child and must
+    // not be swept up by a run close.
+    const closeSession = vi.fn().mockResolvedValue(true)
+    await closeOrchestrationRun({
+      state: stateWith({ agent: child(), shell: child({ kind: 'terminal' }) }),
+      parentSessionId: PARENT,
+      closeSession,
+    })
+    expect(closeSession.mock.calls.map(c => c[0])).toEqual(['agent'])
+  })
+
+  it('returns an empty result when the caller has no children', async () => {
+    const result = await closeOrchestrationRun({
+      state: stateWith({}),
+      parentSessionId: PARENT,
+      closeSession: vi.fn().mockResolvedValue(true),
+    })
+    expect(result.closedSessionIds).toEqual([])
+    expect('skippedSessionIds' in result).toBe(false)
+  })
+
+  it('only touches the caller’s own children', async () => {
+    const closeSession = vi.fn().mockResolvedValue(true)
+    await closeOrchestrationRun({
+      state: stateWith({
+        mine: child(),
+        theirs: child({ orchestrationParentId: 'other' as SessionId, orchestrationRootId: 'other' as SessionId }),
+      }),
+      parentSessionId: PARENT,
+      closeSession,
+    })
+    expect(closeSession.mock.calls.map(c => c[0])).toEqual(['mine'])
+  })
+})