From ef30c09dd06dd018f1322322c20dd147eee4a351 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 18:15:11 +0200 Subject: [PATCH 1/2] feat(orchestration): stop confirming closes of an agent's own children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An orchestrating parent closing the children it spawned was raising a confirmation dialog — once per close_agent, and once per run on close_run. Those agents are disposable by construction: the parent created them for a task, and closing them when the task is done is the normal end of their lifecycle, not a destructive act on something the user built. A dialog that fires on routine housekeeping is how "are you sure?" stops meaning anything by the time it guards something that matters. The safety property was never the dialog. It is the ownership gate: isVisibleToOrchestrationParent restricts every orchestration read and close to children the caller itself created, or descendants of its own root run. That gate is untouched, and the WHY on OrchestrationCloseSession now says that if it is ever loosened this decision has to be revisited with it. Agent Management MCP is deliberately NOT changed. Its blast radius is every agent in the caller's project rather than only its own children, so a human still signs off there — which is also what the README's "destructive close is restricted to an explicit current user request" describes. This also fixes the reporting bug PR #625's review surfaced. The old run loop pushed every id into closedSessionIds and relied on a decline THROWING to reach its catch — but the confirmation gate resolves false, it never rejected, so skippedSessionIds could not populate from a decline and the parent was told agents closed that had not. closeOrchestrationAgent was worse: it returned the id unconditionally with no branch at all. Both now branch on closeSession's boolean. With no dialog a decline is impossible, but an already-gone session still returns false and is correctly reported as skipped. The two wrapper adapters added in #625 to drop that boolean for a Promise contract are gone with the contract; OrchestrationCloseSession returns Promise and no longer carries requireConfirmation. Co-Authored-By: Claude Opus 5 (1M context) --- src/renderer/src/workspace/hook/index.ts | 25 +-- .../src/workspace/orchestrationClose.test.ts | 153 ++++++++++++++++++ .../src/workspace/orchestrationMcp.ts | 74 +++++---- 3 files changed, 198 insertions(+), 54 deletions(-) create mode 100644 src/renderer/src/workspace/orchestrationClose.test.ts diff --git a/src/renderer/src/workspace/hook/index.ts b/src/renderer/src/workspace/hook/index.ts index 94470c92..04c6baf3 100644 --- a/src/renderer/src/workspace/hook/index.ts +++ b/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. - // - // 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, diff --git a/src/renderer/src/workspace/orchestrationClose.test.ts b/src/renderer/src/workspace/orchestrationClose.test.ts new file mode 100644 index 00000000..2a314fbf --- /dev/null +++ b/src/renderer/src/workspace/orchestrationClose.test.ts @@ -0,0 +1,153 @@ +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 confirmation dialog these calls used to raise is gone (see the WHY on +// OrchestrationCloseSession), so `preConfirmed: true` is now part of the +// contract too — a regression that re-armed the dialog would make a fleet's +// routine cleanup interrupt the user several times per run. + +const PARENT = 'parent-1' as SessionId + +const child = (overrides: Partial = {}): SessionMeta => ({ + cwd: '/tmp/project', + kind: 'claude', + orchestrationParentId: PARENT, + orchestrationRootId: PARENT, + ...overrides, +}) as SessionMeta + +const stateWith = (sessions: Record): 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, + }) + // preConfirmed is the whole point: these are the caller's own children and + // the ownership gate already scoped them. + expect(closeSession).toHaveBeenCalledWith('child-1', { preConfirmed: true }) + }) + + 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('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. Now none of them do. + for (const call of closeSession.mock.calls) { + expect(call[1]).toEqual({ preConfirmed: true }) + } + }) + + 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('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), + }) + expect(result.skippedSessionIds).toBeUndefined() + }) + + 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']) + }) +}) diff --git a/src/renderer/src/workspace/orchestrationMcp.ts b/src/renderer/src/workspace/orchestrationMcp.ts index 2956701b..6efcbb6d 100644 --- a/src/renderer/src/workspace/orchestrationMcp.ts +++ b/src/renderer/src/workspace/orchestrationMcp.ts @@ -117,22 +117,30 @@ export function readOrchestrationRunOutputs(params: { /** * How `closeSession` is called from the orchestration MCP surface. * - * Typed here rather than inlined so the options — specifically - * `requireConfirmation` — cannot be dropped by a caller that copies the older - * one-argument shape. + * WHY orchestration closes are NOT confirmed, while Agent Management's are: + * the two surfaces have different blast radii. Agent Management can reach any + * agent in the caller's project, so a human has to sign off. Orchestration + * cannot — `isVisibleToOrchestrationParent` restricts every read and close to + * children the caller itself created (or descendants of its own root run). + * Those agents are disposable by construction: the parent spawned them for a + * task, and closing them when the task is done is the normal end of their + * lifecycle, not a destructive act on something the user built. + * + * The dialog was therefore asking a human to approve a fleet cleaning up after + * itself, several times per run. A confirmation that fires on routine + * housekeeping is the reason "are you sure?" stops meaning anything by the time + * it guards something that matters. + * + * The ownership gate is what keeps this safe and is deliberately untouched. If + * that gate is ever loosened, this decision has to be revisited with it. + * + * Returns whether the session actually closed, so a caller can report an + * already-gone agent as skipped rather than claiming it closed one. */ export type OrchestrationCloseSession = ( sessionId: SessionId, - options?: { preConfirmed?: boolean; requireConfirmation?: { headline: string } }, -) => Promise - -/** "Agent “Reviewer” is asking to close an agent it started." — the sentence - * that tells the user why a dialog they did not summon just appeared. */ -function closeRequestHeadline(state: WorkspaceState, parentSessionId: string): string { - const title = state.sessions[parentSessionId]?.title - const who = title ? `Agent “${title}”` : 'An agent' - return `${who} is asking to close an agent it started.` -} + options?: { preConfirmed?: boolean }, +) => Promise export async function closeOrchestrationAgent(params: { state: WorkspaceState @@ -144,10 +152,13 @@ export async function closeOrchestrationAgent(params: { if (!meta || !isVisibleToOrchestrationParent(meta, params.parentSessionId)) { throw new Error('Orchestration agent not found for this parent session.') } - await params.closeSession(params.sessionId, { - requireConfirmation: { headline: closeRequestHeadline(params.state, params.parentSessionId) }, - }) - return { closedSessionIds: [params.sessionId] } + const closed = await params.closeSession(params.sessionId, { preConfirmed: true }) + // Report what happened. Previously this returned the id unconditionally, so + // an agent whose close did not take (already gone) was told it had closed + // one — and it would then proceed on that assumption. + return closed + ? { closedSessionIds: [params.sessionId] } + : { closedSessionIds: [], skippedSessionIds: [params.sessionId] } } export async function closeOrchestrationRun(params: { @@ -163,23 +174,24 @@ export async function closeOrchestrationRun(params: { ) const closedSessionIds: string[] = [] const skippedSessionIds: string[] = [] - const headline = closeRequestHeadline(params.state, params.parentSessionId) - for (const [index, sessionId] of sessionIds.entries()) { + for (const sessionId of sessionIds) { try { - // Confirm ONCE, on the first agent, and let the rest ride that answer. + // No dialog, per the WHY on OrchestrationCloseSession. A run close is a + // fleet cleaning up its own children; the ownership gate above already + // guarantees these are the caller's. // - // Forcing per-agent confirmation on a run of twelve would produce twelve - // dialogs, and a user clicking through twelve dialogs is not confirming - // anything — it is the reason "are you sure?" stopped meaning anything. - // The first dialog names the run's requester; declining it throws, which - // the catch below turns into a skip, and the remaining agents then hit - // the same declined gate rather than dying silently. - await params.closeSession(sessionId, { - requireConfirmation: index === 0 ? { headline } : undefined, - preConfirmed: index > 0, - }) - closedSessionIds.push(sessionId) + // The result is branched on rather than assumed. The old loop pushed to + // closedSessionIds unconditionally and relied on a decline THROWING to + // reach the catch — but the confirmation gate resolves false, it never + // rejected, so skippedSessionIds could not populate from a decline and + // the caller was told every agent closed. With no dialog a decline is + // impossible, but an already-gone session still returns false and is now + // correctly reported as skipped. + const closed = await params.closeSession(sessionId, { preConfirmed: true }) + if (closed) closedSessionIds.push(sessionId) + else skippedSessionIds.push(sessionId) } catch { + // A genuine throw (backend kill failure) is still a skip. skippedSessionIds.push(sessionId) } } From c379ed51dbd9b4ebc1350114e003796382ecd473 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 18:43:30 +0200 Subject: [PATCH 2/2] fix(orchestration): confirm only when a close reaches past the named agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers returned BLOCK on the same finding, and they were right. The safety argument was wrong. The ownership gate scopes WHICH SESSION MAY BE NAMED, not WHICH SESSIONS DIE — closeSession kills a set the gate never sees. Two shapes reach further than advertised: - 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. Asserting preConfirmed destroyed a hand-built session silently, and it did not even appear in closedSessionIds. - Closing a tab's sole grid leaf takes every detached session in that tab. Orchestration children live detached in the root's tab, so a close could take the caller itself, its siblings, and Dispatch agents the user parked there — while reporting one id. The decisive evidence: Agent Management already refuses both shapes outright via additionalCloseImpact, *even though it has a dialog available*. Orchestration's only equivalent protection was the dialog this PR removed. So the fix is not to restore the dialog but to scope it. A new silentIfSoleTarget option resolves inside closeSession, where paneCloseTargets is in scope — the only code that computes the full set a close destroys. The routine case (a detached child expanding to exactly itself) stays silent, which is the entire point of the PR; the two reaching shapes fall back to a dialog naming the requester. CloseSessionOptions' doc, which calls preConfirmed an ASSERTION and "deliberately NOT a convenience for this close feels safe", now says why orchestration does not qualify. Also fixed: Agent Management reported a DECLINED close as a success. Its own comment said "declining rejects the tool call" — it did not; the gate resolves false. That is the one surface where a decline is still possible, so it is where the lie mattered most. close_agent gained the catch close_run already had, since a rejected backend kill runs after closeLinkedChildren and could report neither closed nor skipped with children already gone. Test gaps the reviewers found: the runId filter had zero coverage and is now close_run's sole blast-radius control; non-agent kinds and the empty-run case were unpinned; toBeUndefined cannot prove omission; and stateWith's cast through unknown violated docs/testing/standard.md without the harness comment that standard requires. All closed. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/workspace/hook/actions/pane.ts | 56 ++++++++- src/renderer/src/workspace/hook/index.ts | 13 +- .../src/workspace/orchestrationClose.test.ts | 119 ++++++++++++++++-- .../src/workspace/orchestrationMcp.ts | 48 +++++-- 4 files changed, 216 insertions(+), 20 deletions(-) diff --git a/src/renderer/src/workspace/hook/actions/pane.ts b/src/renderer/src/workspace/hook/actions/pane.ts index c8be9344..e6b9193d 100644 --- a/src/renderer/src/workspace/hook/actions/pane.ts +++ b/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) diff --git a/src/renderer/src/workspace/hook/index.ts b/src/renderer/src/workspace/hook/index.ts index 04c6baf3..65b73905 100644 --- a/src/renderer/src/workspace/hook/index.ts +++ b/src/renderer/src/workspace/hook/index.ts @@ -759,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, diff --git a/src/renderer/src/workspace/orchestrationClose.test.ts b/src/renderer/src/workspace/orchestrationClose.test.ts index 2a314fbf..8d277846 100644 --- a/src/renderer/src/workspace/orchestrationClose.test.ts +++ b/src/renderer/src/workspace/orchestrationClose.test.ts @@ -12,10 +12,16 @@ import type { SessionId, SessionMeta, WorkspaceState } from '@renderer/workspace // `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 confirmation dialog these calls used to raise is gone (see the WHY on -// OrchestrationCloseSession), so `preConfirmed: true` is now part of the -// contract too — a regression that re-armed the dialog would make a fleet's -// routine cleanup interrupt the user several times per run. +// 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 @@ -27,6 +33,21 @@ const child = (overrides: Partial = {}): SessionMeta => ({ ...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): WorkspaceState => ({ sessions } as unknown as WorkspaceState) @@ -39,9 +60,12 @@ describe('closeOrchestrationAgent', () => { sessionId: 'child-1', closeSession, }) - // preConfirmed is the whole point: these are the caller's own children and - // the ownership gate already scoped them. - expect(closeSession).toHaveBeenCalledWith('child-1', { preConfirmed: true }) + // 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 () => { @@ -67,6 +91,20 @@ describe('closeOrchestrationAgent', () => { 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. @@ -95,9 +133,13 @@ describe('closeOrchestrationRun', () => { }) expect(result.closedSessionIds.sort()).toEqual(['c1', 'c2', 'c3']) // Previously the FIRST call carried requireConfirmation and the rest rode - // that answer. Now none of them do. + // 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({ preConfirmed: true }) + expect(call[1]).toEqual({ + silentIfSoleTarget: { headline: expect.stringContaining('asking to close') }, + }) } }) @@ -112,6 +154,21 @@ describe('closeOrchestrationRun', () => { 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) => { @@ -135,7 +192,49 @@ describe('closeOrchestrationRun', () => { parentSessionId: PARENT, closeSession: vi.fn().mockResolvedValue(true), }) - expect(result.skippedSessionIds).toBeUndefined() + // `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), + a2: child({ orchestrationRunId: 'run-a' } as Partial), + b1: child({ orchestrationRunId: 'run-b' } as Partial), + }), + 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 () => { diff --git a/src/renderer/src/workspace/orchestrationMcp.ts b/src/renderer/src/workspace/orchestrationMcp.ts index 6efcbb6d..a24d3cde 100644 --- a/src/renderer/src/workspace/orchestrationMcp.ts +++ b/src/renderer/src/workspace/orchestrationMcp.ts @@ -131,17 +131,33 @@ export function readOrchestrationRunOutputs(params: { * housekeeping is the reason "are you sure?" stops meaning anything by the time * it guards something that matters. * - * The ownership gate is what keeps this safe and is deliberately untouched. If - * that gate is ever loosened, this decision has to be revisited with it. + * WHY `silentIfSoleTarget` and not `preConfirmed`: the ownership gate below + * scopes WHICH SESSION MAY BE NAMED, not WHICH SESSIONS DIE. `closeSession` + * kills a set, and two shapes reach past the named target — a linked agent the + * USER attached to a child (it carries no orchestration fields, so the gate + * cannot see it), and a tab's sole grid leaf (which takes every detached + * session in that tab, including the caller's siblings and the user's parked + * agents). Asserting preConfirmed would destroy those silently. This mode stays + * silent for the routine case — a detached child that expands to exactly + * itself — and falls back to a dialog naming the requester for the rest. * * Returns whether the session actually closed, so a caller can report an * already-gone agent as skipped rather than claiming it closed one. */ export type OrchestrationCloseSession = ( sessionId: SessionId, - options?: { preConfirmed?: boolean }, + options?: { silentIfSoleTarget?: { headline: string } }, ) => Promise +/** "Agent “Reviewer” is asking to close an agent it started." — the sentence + * that tells the user why a dialog they did not summon just appeared. Only + * reached when the close would reach past the agent that was named. */ +function closeRequestHeadline(state: WorkspaceState, parentSessionId: string): string { + const title = state.sessions[parentSessionId]?.title + const who = title ? `Agent “${title}”` : 'An agent' + return `${who} is asking to close an agent it started.` +} + export async function closeOrchestrationAgent(params: { state: WorkspaceState parentSessionId: string @@ -152,10 +168,17 @@ export async function closeOrchestrationAgent(params: { if (!meta || !isVisibleToOrchestrationParent(meta, params.parentSessionId)) { throw new Error('Orchestration agent not found for this parent session.') } - const closed = await params.closeSession(params.sessionId, { preConfirmed: true }) + const closed = await params.closeSession(params.sessionId, { + silentIfSoleTarget: { + headline: closeRequestHeadline(params.state, params.parentSessionId), + }, + }).catch(() => false) // Report what happened. Previously this returned the id unconditionally, so // an agent whose close did not take (already gone) was told it had closed // one — and it would then proceed on that assumption. + // The `.catch(() => false)` above mirrors close_run's catch: a rejected + // backend kill runs AFTER closeLinkedChildren, so a bare throw could report + // neither closed nor skipped while children were already gone. return closed ? { closedSessionIds: [params.sessionId] } : { closedSessionIds: [], skippedSessionIds: [params.sessionId] } @@ -174,6 +197,7 @@ export async function closeOrchestrationRun(params: { ) const closedSessionIds: string[] = [] const skippedSessionIds: string[] = [] + const headline = closeRequestHeadline(params.state, params.parentSessionId) for (const sessionId of sessionIds) { try { // No dialog, per the WHY on OrchestrationCloseSession. A run close is a @@ -185,9 +209,19 @@ export async function closeOrchestrationRun(params: { // reach the catch — but the confirmation gate resolves false, it never // rejected, so skippedSessionIds could not populate from a decline and // the caller was told every agent closed. With no dialog a decline is - // impossible, but an already-gone session still returns false and is now - // correctly reported as skipped. - const closed = await params.closeSession(sessionId, { preConfirmed: true }) + // impossible, but a session that is simply gone still returns false and + // is now correctly reported as skipped. + // + // Be precise about what false means: closeSession returns it whenever the + // id is in neither `tabs` nor `detachedSessions`. The id list here is + // computed once from a pre-loop snapshot, so a sibling taken out by an + // EARLIER iteration's tab cascade also reports false — i.e. "skipped" can + // include "already closed by this very operation". The error direction is + // the safe one (it over-reports survival, where the old code + // over-reported success), but do not read skipped as "still running". + const closed = await params.closeSession(sessionId, { + silentIfSoleTarget: { headline }, + }) if (closed) closedSessionIds.push(sessionId) else skippedSessionIds.push(sessionId) } catch {