From 37603ec532f80cbb1caa9fcbdcdca05814aefb53 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Sun, 6 Sep 2026 03:14:16 -0700 Subject: [PATCH] fix(orchestration): close the review findings on the structured parity work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects and two follow-ups from the delta review. The `worktree rm` refusal was a dead end in the desktop UI. Its message matched no matcher in `classifyWorktreeForceDeleteReason`, and an ordinary desktop delete already passes `force=true` for the dirty-file skip, so classification returned null unconditionally: the toast showed raw CLI wording with no Force Delete button, and a user with a live chat session was stuck unless they knew to reach for the CLI. That is the #11960 shape `shared/worktree/removal.ts` documents, so the refusal now has its own prefix, matcher, `WorktreeForceDeleteReason` and toast copy, classified BEFORE the `force` guard and nulled once the waiver is spent — exactly how `unstopped-pty` is handled, with matcher and hint kept in the same file as that contract requires. The copy says Force Delete will close a running conversation rather than borrowing the "could not confirm" wording, because Orca watched these sessions stay attached; there is no doubt to waive. Structured `terminal read` cursors were unsound and are now refused. The PTY cursor indexes an append-only completed-line buffer with a monotone count; a session journal is a BOUNDED tail re-projected on every read, so a saved index addressed different lines as the journal grew — and `truncated` could never fire to say so, because it tests `cursor < oldestCursor` and `oldestCursor` was always 0. A poller got wrong or duplicated lines under `truncated:false`. Separately, a streaming turn's lines counted as completed with `partialLine` hardcoded empty, so a mid-turn cursor consumed a half-written line whose growth was never redelivered — the `"hel"`/`"hello"` hazard the PTY reader guards against. The journal does have stable item identity, but `terminal.read`'s cursor is a number on the wire and cannot carry it, so a cursor read now refuses and names `worker-read --source transcript`, which already has that contract including `source_changed`. No cursor space is advertised either: `nextCursor` is null and the cursor fields are absent, rather than claiming an index the next read cannot honour. The header claim that all four fields kept their meanings was true of the shape and false of the invariants; it now says which ones hold. Two fixes had no test at their real seam, which is the same failure that produced this whole set — the runtime tested directly, the seam tested by neither. The group-addressing test hand-composed the recipient list itself, so deleting the composition at the call site left it green; it now drives `sendGroupMessage` with no PTY terminals at all. Nothing referenced `isLiveStructuredAgent`, so the `dispatch --inject` fix had no red-then-green at all; it now has one driving `RuntimeTerminalAgentPresence.isRunning`. Both were ablated and confirmed red. Folder-workspace removals sweep and kill PTYs without `requirePhysicalStop`, so the structured sweep no-opped there and left a live session bound to a workspace about to be forgotten. They now close best-effort under an explicit `closeStructuredSessions` flag, kept separate from `requirePhysicalStop` because the two questions differ: that one asks whether a stop must be PROVEN before files are touched, and it is what licenses a refusal. These paths do not refuse — the root is shared so no checkout vanishes under the child, and one of them is a never-throw forget a refusal would wedge. Reconciliation sweeps set neither and still close nothing. Also: the force close is raced against the same sweep deadline every PTY surface is bounded by, so a wedged provider close reports the timeout instead of hanging `worktree rm --force` forever; and the refusal now prints a count and the providers instead of raw session ids, which our own marker rationale treats as one tab-id hop from a credential. --- .../removal/remove-folder-workspace.ts | 3 ++ .../runtime/folder-workspace-pty-teardown.ts | 3 ++ ...untime-remove-orphan-or-folder-worktree.ts | 3 ++ ...structured-worker-group-addressing.test.ts | 54 +++++++++++++++++++ ...ructured-session-worktree-teardown.test.ts | 44 +++++++++++++++ .../structured-session-worktree-teardown.ts | 18 +++++-- .../structured-worker-agent-presence.test.ts | 46 ++++++++++++++++ .../structured-worker-terminal-read.test.ts | 28 +++++++--- .../structured-worker-terminal-read.ts | 42 ++++++++++++--- src/main/runtime/worktree-teardown.ts | 53 ++++++++++++++---- .../sidebar/delete-worktree-toast.ts | 17 ++++++ src/shared/worktree/removal.ts | 18 +++++++ 12 files changed, 300 insertions(+), 29 deletions(-) create mode 100644 src/main/runtime/structured-worker-agent-presence.test.ts diff --git a/src/main/ipc/worktrees/removal/remove-folder-workspace.ts b/src/main/ipc/worktrees/removal/remove-folder-workspace.ts index c03b1449136..916f4574a92 100644 --- a/src/main/ipc/worktrees/removal/remove-folder-workspace.ts +++ b/src/main/ipc/worktrees/removal/remove-folder-workspace.ts @@ -43,6 +43,9 @@ export async function removeFolderWorkspace( : {}), localProvider: sshPtyProvider ?? getLocalPtyProvider(), onPtyStopped: clearProviderPtyState, + // A structured session is on no PTY surface, so the sweeps above leave it attached to a + // workspace this removal is about to forget. Closed best-effort, exactly like those PTYs. + closeStructuredSessions: true, ...(externalHost ? { includeProviderInventory: ownerHost?.kind === 'ssh' && Boolean(sshPtyProvider), diff --git a/src/main/runtime/folder-workspace-pty-teardown.ts b/src/main/runtime/folder-workspace-pty-teardown.ts index f5a5b7df4b3..e54c9165c46 100644 --- a/src/main/runtime/folder-workspace-pty-teardown.ts +++ b/src/main/runtime/folder-workspace-pty-teardown.ts @@ -29,6 +29,9 @@ export async function teardownFolderWorkspacePtys( ...(connectionId ? { resolvedConnectionId: connectionId } : {}), localProvider: ptyProvider, onPtyStopped: deps.onPtyStopped ?? undefined, + // A structured session is on no PTY surface, so the sweeps above leave it attached to a + // workspace this removal is about to forget. Closed best-effort, exactly like those PTYs. + closeStructuredSessions: true, ...(connectionId ? { includeProviderInventory: Boolean(sshPtyProvider), includeLocalRegistry: false } : {}) diff --git a/src/main/runtime/orca-runtime-remove-orphan-or-folder-worktree.ts b/src/main/runtime/orca-runtime-remove-orphan-or-folder-worktree.ts index 01f4f305803..6c011014b6c 100644 --- a/src/main/runtime/orca-runtime-remove-orphan-or-folder-worktree.ts +++ b/src/main/runtime/orca-runtime-remove-orphan-or-folder-worktree.ts @@ -44,6 +44,9 @@ export async function removeOrphanOrFolderWorktree({ : {}), localProvider: ptyProvider, onPtyStopped: runtime.onPtyStopped ?? undefined, + // A structured session is on no PTY surface, so the sweeps above leave it attached to a + // workspace this removal is about to forget. Closed best-effort, exactly like those PTYs. + closeStructuredSessions: true, ...(externalOrphanHost ? { includeProviderInventory: orphanHost?.kind === 'ssh' && Boolean(sshPtyProvider), diff --git a/src/main/runtime/orchestration/structured-worker-group-addressing.test.ts b/src/main/runtime/orchestration/structured-worker-group-addressing.test.ts index 7eff64bc61f..604c3d1f13d 100644 --- a/src/main/runtime/orchestration/structured-worker-group-addressing.test.ts +++ b/src/main/runtime/orchestration/structured-worker-group-addressing.test.ts @@ -11,6 +11,7 @@ vi.mock('../../native-chat/agent-session-wire/structured-agent-session-registry' const { listAddressableStructuredWorkers, structuredWorkerAgentStatus } = await import('./structured-worker-group-addressing') const { resolveGroupAddress } = await import('./groups') +const { sendGroupMessage } = await import('../rpc/methods/orchestration-send-group') const { mintStructuredWorkerHandle, mintStructuredWorkerPaneKey, @@ -164,3 +165,56 @@ describe('group addressing and structured workers', () => { expect(structuredWorkerAgentStatus(SESSION_ID)).toBeNull() }) }) + +describe('sendGroupMessage actually composes structured workers in', () => { + beforeEach(() => { + structuredWorkerIdentities.clear() + hostRef.current = null + }) + + /** + * Drives the real `sendGroupMessage`, not `resolveGroupAddress`. + * + * The suite above hand-composed `[PTY_TERMINAL, ...listAddressableStructuredWorkers()]` itself, + * so deleting the composition at the call site left it green — the exact regression the fix + * describes could come straight back. This test owns that seam. + */ + it('addresses a structured worker that only the call site can enumerate', async () => { + const handle = registerWorker() + installHost({}) + const inserted: { to: string }[] = [] + const db = { + getLegacyAdoptedRunMailboxOwner: () => null, + getCurrentRunForPane: () => undefined, + getActiveDispatchMailboxOwners: () => [], + getRunMailboxOwnerIdsForHandle: () => [], + insertMessages: (rows: { to: string }[]) => { + inserted.push(...rows) + return rows.map((row, index) => ({ id: `m${index}`, to_handle: row.to, type: 'status' })) + } + } + const runtime = { + // No PTY terminals at all: if the call site does not compose structured workers in, the + // group resolves empty and this throws instead of delivering. + listTerminals: async () => ({ terminals: [] }), + getAgentStatusForHandle: () => 'idle', + getLiveTerminalPaneKey: () => structuredWorkerIdentities.get(handle)!.paneKey, + notifyMessageArrived: () => {} + } + await sendGroupMessage({ + params: { subject: 's', body: 'b', type: 'status', priority: 'normal' }, + runtime: runtime as never, + db: db as never, + from: 'term_sender', + groupAddress: '@all', + senderPaneKey: undefined, + senderRunId: undefined, + explicitRunId: undefined, + legacyCoordinatorRunId: undefined, + revalidateLegacyCoordinator: undefined, + recordMutationReceipt: undefined, + withSendWarnings: (receipt) => receipt + } as never) + expect(inserted.map((row) => row.to)).toEqual([handle]) + }) +}) diff --git a/src/main/runtime/structured-session-worktree-teardown.test.ts b/src/main/runtime/structured-session-worktree-teardown.test.ts index d3af1d0300c..53c9f126387 100644 --- a/src/main/runtime/structured-session-worktree-teardown.test.ts +++ b/src/main/runtime/structured-session-worktree-teardown.test.ts @@ -8,6 +8,7 @@ vi.mock('../native-chat/agent-session-wire/structured-agent-session-registry', ( })) const { killAllProcessesForWorktree } = await import('./worktree-teardown') +const { classifyWorktreeForceDeleteReason } = await import('../../shared/worktree/removal') const { listLiveStructuredSessionsForWorktree } = await import('./structured-session-worktree-teardown') @@ -99,6 +100,49 @@ describe('worktree teardown and structured agent sessions', () => { await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).rejects.toThrow(/force/i) }) + it('classifies for the desktop Force Delete button, not just the CLI', async () => { + // The #11960 dead end, and the shape this file's own comments warn about: the desktop + // affordance comes ONLY from the classifier, and an ordinary delete already passes force:true + // for the dirty-file skip — so a refusal with no matcher shows raw CLI wording with no button. + installHost({ records: [record('s1', WORKTREE)] }) + const error = await killAllProcessesForWorktree(WORKTREE, destructiveDeps()).catch( + (thrown: Error) => thrown.message + ) + expect(classifyWorktreeForceDeleteReason(error as string, true)).toBe('running-agent-session') + // Nulled once the waiver is spent, exactly as `unstopped-pty` is, so the button does not + // reappear on a delete the user already forced. + expect(classifyWorktreeForceDeleteReason(error as string, true, true)).toBeNull() + }) + + it('keeps session ids out of a message users and agents read', async () => { + // A session id is one tab-id hop from the random pane key that gates a worker's mailbox, and + // this string reaches CLI output and a desktop toast. A count and the providers are what a + // user deciding whether to force actually needs. + installHost({ records: [record('s1', WORKTREE)] }) + const error = await killAllProcessesForWorktree(WORKTREE, destructiveDeps()).catch( + (thrown: Error) => thrown.message + ) + expect(error).not.toContain('s1') + expect(error).toContain('1 running agent session') + }) + + it('closes best-effort for a folder-workspace removal, which requires no stop proof', async () => { + // Those paths sweep and kill PTYs without `requirePhysicalStop`, so the structured sweep used + // to no-op there and left a live session bound to a workspace Orca was about to forget. They + // do not refuse: the root is shared so no checkout vanishes, and one of them is a never-throw + // forget that a refusal would wedge. + const host = installHost({ records: [record('s1', WORKTREE)] }) + await expect( + killAllProcessesForWorktree(WORKTREE, { + localProvider, + includeProviderInventory: false, + includeLocalRegistry: false, + closeStructuredSessions: true + }) + ).resolves.toMatchObject({ structuredStopped: 1 }) + expect(host.closed).toEqual(['s1']) + }) + it('closes them under force instead of orphaning the child', async () => { const host = installHost({ records: [record('s1', WORKTREE), record('s2', WORKTREE)] }) const result = await killAllProcessesForWorktree( diff --git a/src/main/runtime/structured-session-worktree-teardown.ts b/src/main/runtime/structured-session-worktree-teardown.ts index 15801835f89..226f785f039 100644 --- a/src/main/runtime/structured-session-worktree-teardown.ts +++ b/src/main/runtime/structured-session-worktree-teardown.ts @@ -59,14 +59,19 @@ export function listLiveStructuredSessionsForWorktree( .map((record) => ({ sessionId: record.sessionId, agent: record.provider })) } +/** + * Counts and providers, never session ids. + * + * A session id is one tab-id hop from the random pane key that gates a worker's mailbox, and this + * string reaches agent-readable CLI output and a desktop toast. The count and the providers are + * what a user deciding whether to force actually needs; the ids identify nothing they can act on. + */ export function describeLiveStructuredSessions( - worktreeId: string, sessions: readonly LiveStructuredSessionInWorkspace[] ): string { const noun = sessions.length === 1 ? 'agent session' : 'agent sessions' - return `${sessions.length} running ${noun} in ${worktreeId} (${sessions - .map((session) => `${session.agent}:${session.sessionId}`) - .join(', ')})` + const providers = [...new Set(sessions.map((session) => session.agent))].sort().join(', ') + return `${sessions.length} running ${noun} (${providers})` } /** @@ -79,6 +84,11 @@ export async function closeStructuredSessionsForWorktree( worktreeId: string, runtime?: StructuredWorktreeSweepRuntime ): Promise<{ closed: number; unstopped: LiveStructuredSessionInWorkspace[] }> { + // No `afterClose` for a dispatched worker: `host.close` drops the holds, so nothing keeps a + // provider child un-evictable, but the dispatch's redrive subscription and registry entry do + // survive until it settles by another verb. That is a bounded leak, not a hazard — and passing + // one here would mean resolving a dispatch id per session on a teardown path that must stay + // inside the sweep deadline. const sessions = listLiveStructuredSessionsForWorktree(worktreeId) const unstopped: LiveStructuredSessionInWorkspace[] = [] let closed = 0 diff --git a/src/main/runtime/structured-worker-agent-presence.test.ts b/src/main/runtime/structured-worker-agent-presence.test.ts new file mode 100644 index 00000000000..ca31e891cc9 --- /dev/null +++ b/src/main/runtime/structured-worker-agent-presence.test.ts @@ -0,0 +1,46 @@ +/** + * `isTerminalRunningAgent` for a worker that IS a structured agent session. + * + * This seam had no test at all: nothing in the repo referenced `isLiveStructuredAgent`, so the + * early return could be deleted and every suite stayed green. `dispatch --to --inject` + * depends on it — without it `getLiveLeaf` throws, the catch returns false, and a coordinator is + * told its worker is a bare shell (`no_agent_detected`). + */ + +import { describe, expect, it, vi } from 'vitest' +import { RuntimeTerminalAgentPresence } from './runtime-terminal-agent-presence' + +function presence(isLiveStructuredAgent: (handle: string) => boolean) { + const getLiveLeaf = vi.fn(() => { + // Exactly what the runtime does for a handle with no pane, and the reason the catch below + // used to swallow the question into `false`. + throw new Error('terminal_handle_stale') + }) + return { + getLiveLeaf, + presence: new RuntimeTerminalAgentPresence({ + isLiveStructuredAgent, + getLivePty: () => null, + getLiveLeaf: getLiveLeaf as never, + getPrimaryLeaf: () => null, + getTrackedPty: () => null, + getTabTitle: () => null, + getForegroundProcess: () => null + }) + } +} + +describe('agent presence for a structured worker', () => { + it('reports the session as running an agent without probing a pane', async () => { + const { presence: subject, getLiveLeaf } = presence(() => true) + await expect(subject.isRunning('structworker_1')).resolves.toBe(true) + // A structured session IS the agent; there is no foreground process to recognise, and the + // leaf probe would only throw. + expect(getLiveLeaf).not.toHaveBeenCalled() + }) + + it('still answers false for a handle that is not a live structured worker', async () => { + const { presence: subject } = presence(() => false) + await expect(subject.isRunning('term_gone')).resolves.toBe(false) + }) +}) diff --git a/src/main/runtime/structured-worker-terminal-read.test.ts b/src/main/runtime/structured-worker-terminal-read.test.ts index 714357a6c0b..9bad5d46d1e 100644 --- a/src/main/runtime/structured-worker-terminal-read.test.ts +++ b/src/main/runtime/structured-worker-terminal-read.test.ts @@ -87,15 +87,29 @@ describe('reading a structured worker through the terminal-read path', () => { expect(read?.truncated).toBe(false) }) - it('pages by cursor and limit exactly as a PTY read does', () => { + it('honours limit, and claims no cursor space it cannot honour', () => { const handle = registerWorker() installHost({ items: [message('i1', 'a'), message('i2', 'b'), message('i3', 'c')] }) - const first = readStructuredWorkerTerminal({ handle, db: null, cursor: 0, limit: 2 }) - expect(first?.tail).toEqual(['[assistant] a', '[assistant] b']) - expect(first?.nextCursor).toBe('2') - const second = readStructuredWorkerTerminal({ handle, db: null, cursor: 2 }) - expect(second?.tail).toEqual(['[assistant] c']) - expect(second?.latestCursor).toBe('3') + const read = readStructuredWorkerTerminal({ handle, db: null, limit: 2 }) + expect(read?.tail).toEqual(['[assistant] b', '[assistant] c']) + // No index is advertised: the next read re-projects a sliding window, so 0/length would name + // positions that address different lines by then. + expect(read?.nextCursor).toBeNull() + expect(read?.oldestCursor).toBeUndefined() + expect(read?.latestCursor).toBeUndefined() + }) + + it('refuses a cursor read rather than silently misdelivering lines', () => { + // The PTY cursor indexes an append-only completed-line buffer with a monotone count. This + // window is a bounded tail re-projected every read, so a saved index addresses different lines + // as the journal grows — and `truncated` could never fire to say so, because it tests + // `cursor < oldestCursor` and `oldestCursor` was always 0. A poller would get wrong or + // duplicated lines with `truncated:false`. + const handle = registerWorker() + installHost({ items: [message('i1', 'a')] }) + expect(() => readStructuredWorkerTerminal({ handle, db: null, cursor: 0 })).toThrow( + /worker-read --source transcript/ + ) }) it('reports dropped history as truncated rather than pretending the page is whole', () => { diff --git a/src/main/runtime/structured-worker-terminal-read.ts b/src/main/runtime/structured-worker-terminal-read.ts index 691f125175b..a98d846d572 100644 --- a/src/main/runtime/structured-worker-terminal-read.ts +++ b/src/main/runtime/structured-worker-terminal-read.ts @@ -6,9 +6,15 @@ * coordinator standing a peer does not have, so the only agent-to-agent read verb refused to * resolve the handle. This serves the same verb from the session's journal. * - * The result is a plain `RuntimeTerminalRead` — the journal is projected to LINES and paged by the - * very reader the PTY tail uses — so nothing an agent reads reveals which kind of worker answered, - * and cursor, limit, `truncated` and `nextCursor` keep their existing meanings. + * The result is a plain `RuntimeTerminalRead` — the journal is projected to LINES and bounded by + * the very reader the PTY tail uses — so nothing an agent reads reveals which kind of worker + * answered. `limit` and `truncated` keep their existing meanings. + * + * `cursor` does NOT, and is refused rather than approximated. The PTY contract is an index into an + * append-only completed-line buffer with a monotone count; a session journal is a bounded tail + * re-projected on every read, so the same index addresses different lines over time and + * `oldestCursor` can never grow to admit it. Pagination with a real anchor lives on + * `worker-read --source transcript`. * * READ ONLY, deliberately. `terminal.show` still refuses a structured handle: synthesising a * `ptyId`/`leafId`/`paneRuntimeId` would hand every public terminal verb something that looks @@ -45,6 +51,20 @@ export function readStructuredWorkerTerminal(args: { if (!identity) { return null } + if (args.cursor !== undefined) { + // A numeric cursor cannot be re-anchored here. `terminal.read`'s cursor is an index into an + // append-only completed-line buffer with a monotone count; this window is a BOUNDED tail that + // is re-projected every read, so the same index means different lines as the journal grows, + // and `truncated` (`cursor < oldestCursor`) could never fire to say so because `oldestCursor` + // is always 0. Serving it would silently return wrong or duplicated lines to a poller. The + // journal does have stable item identity, but the terminal cursor is a number on the wire and + // cannot carry it — `worker-read --source transcript` already has that contract, including + // `source_changed` detection. + throw new Error( + `${args.handle} serves recent output without a cursor; its history is not line-addressable. ` + + 'Read it without --cursor, or page it with `orca orchestration worker-read --source transcript`.' + ) + } const page = readStructuredJournalPage(identity.sessionId) if (!page) { // Honest refusal, and the same one the send lane reports: an empty tail would read as "this @@ -56,18 +76,24 @@ export function readStructuredWorkerTerminal(args: { const lines = bounded.messages.flatMap((message) => formatWorkerTranscriptMessage(message).split('\n') ) - return readTerminalTail({ + const read = readTerminalTail({ handle: args.handle, status: structuredWorkerTerminalState(observeStructuredWorker(identity).status), previewLines: lines, - completedLines: lines, - // Every projected line is complete; a session journal has no half-written trailing line. + // Unreachable without a cursor, and deliberately empty rather than a copy of `lines`: a + // running turn's text is still growing, so calling it "completed" is the `"hel"`/`"hello"` + // hazard the PTY reader guards against. + completedLines: [], partialLine: '', - completedLineCount: lines.length, + completedLineCount: 0, // Older items really were dropped, by the page limit or the byte bound; `truncated` is how the // PTY read already says exactly that. bufferTruncated: page.hasOlder || bounded.limited, - ...(args.cursor === undefined ? {} : { cursor: args.cursor }), ...(args.limit === undefined ? {} : { limit: args.limit }) }) + // No cursor space is claimed, because none exists here. `nextCursor: null` is the contract's own + // "nothing to continue from"; emitting 0/length would advertise an index the next read cannot + // honour. + const { oldestCursor: _oldest, latestCursor: _latest, ...withoutCursorSpace } = read + return { ...withoutCursorSpace, nextCursor: null } } diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 1ce60d3ee65..82fa055cc3a 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -2,6 +2,8 @@ import type { IPtyProvider } from '../providers/types' import type { OrcaRuntimeService } from './orca-runtime' import { isUnstoppedPtyRemovalError, + RUNNING_AGENT_SESSION_REMOVAL_PREFIX, + UNSTOPPED_PTY_DETAIL_SEPARATOR, WORKTREE_TEARDOWN_FORCE_HINT, WORKTREE_TEARDOWN_TIMEOUT_PREFIX } from '../../shared/worktree/removal' @@ -40,6 +42,15 @@ export type WorktreeTeardownDeps = { allowUnverifiedStop?: boolean includeProviderInventory?: boolean includeLocalRegistry?: boolean + /** + * Close structured agent sessions best-effort, for a destructive removal that does NOT require + * PTY-stop proof — the folder-workspace paths, which sweep and kill PTYs the same way. + * + * Separate from `requirePhysicalStop` because the two questions are different: that one asks + * whether a stop must be PROVEN before files are touched, and it is what licenses a refusal. + * Reconciliation sweeps set neither; they repair state and must never close anything. + */ + closeStructuredSessions?: boolean } export type WorktreeTeardownResult = { @@ -92,7 +103,7 @@ export async function killAllProcessesForWorktree( // of the three surfaces below, so all three answered zero and removal deleted the checkout out // from under a running provider child. Refusing costs nothing when there are none, and the check // is synchronous, so a destructive removal fails fast instead of after the whole sweep budget. - const structuredStopped = await sweepStructuredSessions(worktreeId, deps) + const structuredStopped = await sweepStructuredSessions(worktreeId, deps, deadline, deadlineError) const sweeps = createWorktreeSweepTracker() const stopAttempts = new Map>() const stopPty = ( @@ -243,7 +254,10 @@ export async function killAllProcessesForWorktree( } } else { const summary = describeUnstoppedPtys(worktreeId, failedPtyIds, verdict) - if (!deps.allowUnverifiedStop) { + // Only a proof-requiring removal may refuse. A folder-workspace removal shares its root, so no + // checkout disappears under the child — the harm is a session left pointing at a workspace Orca + // has forgotten — and one of those paths is a never-throw forget, which a refusal would wedge. + if (deps.requirePhysicalStop && !deps.allowUnverifiedStop) { throw new Error(`${summary}. ${WORKTREE_TEARDOWN_FORCE_HINT}`) } // Why: force is the documented escape hatch, so removal continues — but the @@ -271,31 +285,50 @@ export async function killAllProcessesForWorktree( * the same `--force` escape hatch. Force closes them properly instead of orphaning a child against * a `cwd` that is about to disappear. * - * Only the destructive path (`requirePhysicalStop`) participates: the best-effort callers are - * reconciliation sweeps that must never fail a repair, and they delete nothing. + * Two callers participate, for different reasons. A proof-requiring removal (`requirePhysicalStop`) + * refuses, then closes under force. A folder-workspace removal (`closeStructuredSessions`) closes + * best-effort without refusing: it shares its root so no checkout vanishes under the child, and one + * of those paths is a never-throw forget that a refusal would wedge. Reconciliation sweeps set + * neither — they repair state, delete nothing, and must never close a session. */ async function sweepStructuredSessions( worktreeId: string, - deps: WorktreeTeardownDeps + deps: WorktreeTeardownDeps, + deadline: number, + deadlineError: Error ): Promise { - if (!deps.requirePhysicalStop) { + if (!deps.requirePhysicalStop && !deps.closeStructuredSessions) { return 0 } const live = listLiveStructuredSessionsForWorktree(worktreeId) if (live.length === 0) { return 0 } - if (!deps.allowUnverifiedStop) { + // Only a proof-requiring removal may refuse. A folder-workspace removal shares its root, so no + // checkout disappears under the child — the harm is a session left pointing at a workspace Orca + // has forgotten — and one of those paths is a never-throw forget, which a refusal would wedge. + if (deps.requirePhysicalStop && !deps.allowUnverifiedStop) { + // The prefix is what the desktop classifier matches on; without it the toast shows raw CLI + // wording and hides the Force Delete button — the #11960 dead end this file already documents. throw new Error( - `Refusing to remove ${worktreeId}: ${describeLiveStructuredSessions(worktreeId, live)}. ${WORKTREE_TEARDOWN_FORCE_HINT}` + `${RUNNING_AGENT_SESSION_REMOVAL_PREFIX} ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}${describeLiveStructuredSessions(live)}. ${WORKTREE_TEARDOWN_FORCE_HINT}` ) } - const { closed, unstopped } = await closeStructuredSessionsForWorktree(worktreeId, deps.runtime) + // Raced against the same sweep budget every PTY surface is bounded by: `host.close` awaits a + // provider round trip, and a wedged one would otherwise hang `worktree rm --force` forever with + // no timeout error at all. On expiry the force path reports the timeout exactly as the PTY + // sweeps do rather than proceeding as if the sessions had closed. + const { closed, unstopped } = await settleBeforeDeadline( + () => closeStructuredSessionsForWorktree(worktreeId, deps.runtime), + { closed: 0, unstopped: live }, + deadline, + deadlineError + ) if (unstopped.length > 0) { // Force is the documented escape hatch, so removal continues — but say so, because the child // outliving its `cwd` is the failure this sweep exists to make visible. console.warn( - `[worktree-teardown] forcing removal with ${describeLiveStructuredSessions(worktreeId, unstopped)} still attached` + `[worktree-teardown] forcing removal of ${worktreeId} with ${describeLiveStructuredSessions(unstopped)} still attached` ) } return closed diff --git a/src/renderer/src/components/sidebar/delete-worktree-toast.ts b/src/renderer/src/components/sidebar/delete-worktree-toast.ts index 1c013da0da6..946e99eadee 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-toast.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-toast.ts @@ -74,6 +74,23 @@ export function getDeleteWorktreeToastCopy( isDestructive: false } } + if (forceDeleteReason === 'running-agent-session') { + return { + title: translate( + 'auto.components.sidebar.delete.worktree.toast.1d0fa5c0a5', + 'Failed to delete workspace {{value0}}', + { value0: worktreeName } + ), + // Why this is not the "could not confirm" wording: Orca watched these sessions stay + // attached, so there is no doubt to waive — Force Delete ends a conversation that is + // running right now, and any work it holds goes with it. + description: translate( + 'auto.components.sidebar.delete.worktree.toast.runningAgentSession', + 'This workspace still has running agent sessions, so Orca stopped before deleting any files. Force Delete will close them and discard any work they hold.' + ), + isDestructive: false + } + } if (forceDeleteReason === 'missing-registration') { return { title: translate( diff --git a/src/shared/worktree/removal.ts b/src/shared/worktree/removal.ts index 8f5a6c4a4d2..59e5803eddb 100644 --- a/src/shared/worktree/removal.ts +++ b/src/shared/worktree/removal.ts @@ -15,6 +15,7 @@ export type WorktreeForceDeleteReason = | 'orphan-directory' | 'missing-registration' | 'unstopped-pty' + | 'running-agent-session' // Why: everything before this separator is the worktree id — a user-chosen filesystem path. // Only the detail after it is Orca's own wording, so verdict matchers anchor on the boundary @@ -32,6 +33,17 @@ export const UNSTOPPED_PTY_LIVE_DETAIL_PREFIX = 'still live:' // its own matcher the force affordance stayed hidden for the very case it was added for. export const WORKTREE_TEARDOWN_TIMEOUT_PREFIX = 'Timed out waiting for physical PTY teardown:' +// Why (#11960 again): a running agent SESSION blocks removal for the same reason an unstopped PTY +// does, and it needs its own prefix for the same reason the timeout above needed one — the desktop +// force affordance comes only from the classifier below, so a refusal with no matcher shows raw +// CLI wording and hides the Force Delete button. Matcher and hint stay in this file together. +export const RUNNING_AGENT_SESSION_REMOVAL_PREFIX = + 'Refusing to remove worktree with running agent sessions:' + +export function isRunningAgentSessionRemovalError(error: string): boolean { + return error.includes(RUNNING_AGENT_SESSION_REMOVAL_PREFIX) +} + export function isUnstoppedPtyRemovalError(error: string): boolean { return ( error.includes(UNSTOPPED_PTY_REMOVAL_PREFIX) || error.includes(WORKTREE_TEARDOWN_TIMEOUT_PREFIX) @@ -103,6 +115,12 @@ export function classifyWorktreeForceDeleteReason( if (isUnstoppedPtyRemovalError(error)) { return allowUnverifiedPtyStop ? null : 'unstopped-pty' } + // Same placement and the same reason: decided BEFORE the `force` guard, because an ordinary + // desktop delete already passes force:true to skip the dirty-file prompt and that says nothing + // about whether the user has waived closing a live agent session. Only the waiver itself does. + if (isRunningAgentSessionRemovalError(error)) { + return allowUnverifiedPtyStop ? null : 'running-agent-session' + } if (force) { return null }