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 94470c92..65b73905 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, @@ -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, diff --git a/src/renderer/src/workspace/orchestrationClose.test.ts b/src/renderer/src/workspace/orchestrationClose.test.ts new file mode 100644 index 00000000..8d277846 --- /dev/null +++ b/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 => ({ + 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): 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), + 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 () => { + 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..a24d3cde 100644 --- a/src/renderer/src/workspace/orchestrationMcp.ts +++ b/src/renderer/src/workspace/orchestrationMcp.ts @@ -117,17 +117,41 @@ 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. + * + * 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; requireConfirmation?: { headline: string } }, -) => Promise + 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. */ + * 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' @@ -144,10 +168,20 @@ 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, { + 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] } } export async function closeOrchestrationRun(params: { @@ -164,22 +198,34 @@ 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. + // + // 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 a session that is simply gone still returns false and + // is now correctly reported as skipped. // - // 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, + // 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 }, }) - closedSessionIds.push(sessionId) + if (closed) closedSessionIds.push(sessionId) + else skippedSessionIds.push(sessionId) } catch { + // A genuine throw (backend kill failure) is still a skip. skippedSessionIds.push(sessionId) } }