From 5cdfbcf64a04573ebbe448606cdae42c5d8258bf Mon Sep 17 00:00:00 2001 From: Ned Date: Wed, 21 Oct 2026 01:24:10 -0700 Subject: [PATCH 1/2] Preserve new work when an idle Tool stop becomes stale --- docs/specs/dor-tool.md | 2 +- lib/src/components/wall/tool-reaper.test.ts | 34 ++++++++++++++++++++- lib/src/components/wall/tool-reaper.ts | 19 ++++++++++-- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index 43a78e3d2..f0933b173 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -137,7 +137,7 @@ A reap stops an idle Tool's Session, shell included; rehydrate starts it again i 1. Hold the run ([Run end](#run-end)), then write Ctrl+C to the PTY, the graceful-stop signal on every platform; ConPTY delivers it as `CTRL_C_EVENT` (rationale). 2. Keep the last `dehydrate` payload the run emits from then until the kill; none earlier counts. -3. Kill the PTY at the prompt or after a grace, whichever comes first, so a hung Tool blocks nothing. +3. Kill the PTY at the prompt or after a grace, whichever comes first. **Must abandon the kill if the Session or run changed, input arrived, the Tool became dirty, designation ended, or closure took ownership.** **Must rehydrate a reaped Tool when it becomes visible, or when `dor tool` or `dor open` matches it** (reporting `adopted`), never into a closing or transferring Workspace: a fresh shell in the run's directory, the saved command typed as cold restore types it ([Persistence and hosts](#persistence-and-hosts)). It degrades in tiers: diff --git a/lib/src/components/wall/tool-reaper.test.ts b/lib/src/components/wall/tool-reaper.test.ts index 962fd7f1a..676982186 100644 --- a/lib/src/components/wall/tool-reaper.test.ts +++ b/lib/src/components/wall/tool-reaper.test.ts @@ -9,7 +9,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { FakePtyAdapter, setPlatform } from '../../lib/platform'; import type { SpawnPtyOptions } from '../../lib/platform/types'; import { disposeSession, getOrCreateTerminal } from '../../lib/terminal-registry'; -import { registry } from '../../lib/terminal-store'; +import { markSessionTouched, registry } from '../../lib/terminal-store'; import { applyTerminalSemanticEvents, getTerminalPaneState } from '../../lib/terminal-state-store'; import { TerminalProtocolParser } from '../../lib/terminal-protocol'; import { getToolAnnounce, recordToolAnnounce, resetToolAnnounces } from '../../lib/tool-announce-store'; @@ -86,6 +86,7 @@ afterEach(() => { resetToolAnnounces(); resetToolDirty(); resetToolReaps(); + _resetPendingKillsForTesting(); vi.useRealTimers(); }); @@ -195,6 +196,37 @@ describe('stopTool', () => { expect(isToolReaped(ID)).toBe(false); }); + it.each(['new command', 'new input', 'dirty edit', 'replacement Session', 'pending close', 'new finished command', 'released designation'])('abandons a stale stop after %s', async (change) => { + const lath = servingTool(); + const stopped = stopTool(lath, ID); + if (change === 'new command') startRun('important-work'); + if (change === 'new finished command') { + startRun('important-work'); + applyTerminalSemanticEvents(ID, [{ type: 'commandFinish', exitCode: 0 }]); + } + if (change === 'released designation') lath.store.updateParams(ID, { surfaceType: 'terminal' }); + if (change === 'new input') markSessionTouched(ID); + if (change === 'dirty edit') recordToolDirty(ID, true); + if (change === 'pending close') addPendingKill( + { kind: 'surface', id: ID, workspaceId: 'workspace:1' as never, title: 't', label: 'Tool' }, + { restore: () => {}, finalize: () => {} }, + ); + if (change === 'replacement Session') { + disposeSession(ID); + getOrCreateTerminal(ID); + startRun(); + announce(); + kills.length = 0; + } + await finishStop(); + expect(await stopped).toBe(false); + expect(kills).toEqual([]); + expect(registry.has(ID)).toBe(true); + expect(registry.get(ID)?.exited).not.toBe(true); + expect(isToolReaped(ID)).toBe(false); + expect(isToolStopping(ID)).toBe(false); + }); + it('stops nothing that is not eligible', async () => { const lath = servingTool(); announce({ dehydrate: false }); diff --git a/lib/src/components/wall/tool-reaper.ts b/lib/src/components/wall/tool-reaper.ts index a717a4612..31a6daad1 100644 --- a/lib/src/components/wall/tool-reaper.ts +++ b/lib/src/components/wall/tool-reaper.ts @@ -3,7 +3,7 @@ * Tool that declared itself safe to stop, and start it again when it is shown. * `useToolReaper` decides when; this module is what a stop and a rehydrate do. */ -import { holdForHostInterrupt } from '../../lib/tool-run-hold'; +import { holdForHostInterrupt, releaseRunHold } from '../../lib/tool-run-hold'; import { buildShellCommandForKind, shellCommandKind } from 'dor/commands/shell-quote'; import { parseRenderMode } from 'dor-lib-common/browser-providers'; import { getPlatform, PLATFORM_STRING } from '../../lib/platform'; @@ -94,7 +94,10 @@ function waitFor(id: string, done: () => boolean, timeoutMs: number): Promise { if (toolReapBlocker(id, lath.getMeta(id)?.params) !== null) return false; + const entry = registry.get(id)!; + const inputVersion = entry.inputVersion ?? 0; const state = getTerminalPaneState(id); + const runId = state.currentCommand!.id; const cwd = state.currentCommand?.cwdAtStart?.path ?? state.cwd?.path ?? null; beginToolStop(id); let payload: string | null; @@ -107,8 +110,18 @@ export async function stopTool(lath: LathWallEngine, id: string): Promise Date: Wed, 21 Oct 2026 09:54:40 -0700 Subject: [PATCH 2/2] Commit a reap's stop at the interrupt; kill only the Session it interrupted Abandoning the kill after Ctrl+C restored nothing: the run had already exited, so it lapsed the host hold, turned the Tool into a plain terminal, and dropped the captured payload. Keep only the identity check, so a Session reusing the id mid-stop is never killed, and state in the spec that a stop is never abandoned once interrupted. Co-Authored-By: Claude Opus 5.5 --- docs/specs/dor-tool.md | 4 ++- docs/specs/dor-tool.rationale.md | 2 ++ lib/src/components/wall/tool-reaper.test.ts | 30 +++++---------------- lib/src/components/wall/tool-reaper.ts | 21 ++++----------- 4 files changed, 16 insertions(+), 41 deletions(-) diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index f0933b173..06025c1cd 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -137,7 +137,9 @@ A reap stops an idle Tool's Session, shell included; rehydrate starts it again i 1. Hold the run ([Run end](#run-end)), then write Ctrl+C to the PTY, the graceful-stop signal on every platform; ConPTY delivers it as `CTRL_C_EVENT` (rationale). 2. Keep the last `dehydrate` payload the run emits from then until the kill; none earlier counts. -3. Kill the PTY at the prompt or after a grace, whichever comes first. **Must abandon the kill if the Session or run changed, input arrived, the Tool became dirty, designation ended, or closure took ownership.** +3. Kill the PTY at the prompt or after a grace, whichever comes first, so a hung Tool blocks nothing. + +**Never abandon a stop once its Ctrl+C is written**: only a close or a replacement Session cancels the kill, never input, a dirty report, or being shown, which rehydrates once the stop ends (rationale). **Must rehydrate a reaped Tool when it becomes visible, or when `dor tool` or `dor open` matches it** (reporting `adopted`), never into a closing or transferring Workspace: a fresh shell in the run's directory, the saved command typed as cold restore types it ([Persistence and hosts](#persistence-and-hosts)). It degrades in tiers: diff --git a/docs/specs/dor-tool.rationale.md b/docs/specs/dor-tool.rationale.md index dab44e2cf..418092ad2 100644 --- a/docs/specs/dor-tool.rationale.md +++ b/docs/specs/dor-tool.rationale.md @@ -64,6 +64,8 @@ The ladder has no retype (decided 2026-10-03, PR #961 review): no shipped Tool n The stop silences the run's command-exit watch first. Observed in the innerdogfood harness (2026-10-03): a folder viewer launched while engaged had its exit seen, so the reap's Ctrl+C rang `COMMAND_EXIT` while it sat Doored, and the rehydrate carried that ring back as a TODO; a push or spoken alarm would have announced the host's own housekeeping. +The stop commits at the interrupt (decided 2026-10-21, PR #1286 review). By the time the grace ends the run has usually exited, so skipping the kill restores nothing: it lapses the host hold, turns the Surface into a plain terminal, and drops the payload just captured, where finishing leaves a Tool that rehydrates from it. The window is a few seconds on a Tool already out of sight for the idle threshold. + A preview slot is excluded because its retarget interrupts and retypes in place (Preview slot), which a Session with no shell cannot take. A Tool with an auxiliary helper keeps a second shell alive anyway, and killing the parent would orphan it. ## Naming diff --git a/lib/src/components/wall/tool-reaper.test.ts b/lib/src/components/wall/tool-reaper.test.ts index 676982186..fddc89190 100644 --- a/lib/src/components/wall/tool-reaper.test.ts +++ b/lib/src/components/wall/tool-reaper.test.ts @@ -9,7 +9,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { FakePtyAdapter, setPlatform } from '../../lib/platform'; import type { SpawnPtyOptions } from '../../lib/platform/types'; import { disposeSession, getOrCreateTerminal } from '../../lib/terminal-registry'; -import { markSessionTouched, registry } from '../../lib/terminal-store'; +import { registry } from '../../lib/terminal-store'; import { applyTerminalSemanticEvents, getTerminalPaneState } from '../../lib/terminal-state-store'; import { TerminalProtocolParser } from '../../lib/terminal-protocol'; import { getToolAnnounce, recordToolAnnounce, resetToolAnnounces } from '../../lib/tool-announce-store'; @@ -86,7 +86,6 @@ afterEach(() => { resetToolAnnounces(); resetToolDirty(); resetToolReaps(); - _resetPendingKillsForTesting(); vi.useRealTimers(); }); @@ -196,35 +195,18 @@ describe('stopTool', () => { expect(isToolReaped(ID)).toBe(false); }); - it.each(['new command', 'new input', 'dirty edit', 'replacement Session', 'pending close', 'new finished command', 'released designation'])('abandons a stale stop after %s', async (change) => { + it('never kills a replacement Session that reused the id mid-stop', async () => { const lath = servingTool(); const stopped = stopTool(lath, ID); - if (change === 'new command') startRun('important-work'); - if (change === 'new finished command') { - startRun('important-work'); - applyTerminalSemanticEvents(ID, [{ type: 'commandFinish', exitCode: 0 }]); - } - if (change === 'released designation') lath.store.updateParams(ID, { surfaceType: 'terminal' }); - if (change === 'new input') markSessionTouched(ID); - if (change === 'dirty edit') recordToolDirty(ID, true); - if (change === 'pending close') addPendingKill( - { kind: 'surface', id: ID, workspaceId: 'workspace:1' as never, title: 't', label: 'Tool' }, - { restore: () => {}, finalize: () => {} }, - ); - if (change === 'replacement Session') { - disposeSession(ID); - getOrCreateTerminal(ID); - startRun(); - announce(); - kills.length = 0; - } + disposeSession(ID); + getOrCreateTerminal(ID); + startRun(); + kills.length = 0; await finishStop(); expect(await stopped).toBe(false); expect(kills).toEqual([]); - expect(registry.has(ID)).toBe(true); expect(registry.get(ID)?.exited).not.toBe(true); expect(isToolReaped(ID)).toBe(false); - expect(isToolStopping(ID)).toBe(false); }); it('stops nothing that is not eligible', async () => { diff --git a/lib/src/components/wall/tool-reaper.ts b/lib/src/components/wall/tool-reaper.ts index 31a6daad1..a9c5dc765 100644 --- a/lib/src/components/wall/tool-reaper.ts +++ b/lib/src/components/wall/tool-reaper.ts @@ -3,7 +3,7 @@ * Tool that declared itself safe to stop, and start it again when it is shown. * `useToolReaper` decides when; this module is what a stop and a rehydrate do. */ -import { holdForHostInterrupt, releaseRunHold } from '../../lib/tool-run-hold'; +import { holdForHostInterrupt } from '../../lib/tool-run-hold'; import { buildShellCommandForKind, shellCommandKind } from 'dor/commands/shell-quote'; import { parseRenderMode } from 'dor-lib-common/browser-providers'; import { getPlatform, PLATFORM_STRING } from '../../lib/platform'; @@ -94,10 +94,8 @@ function waitFor(id: string, done: () => boolean, timeoutMs: number): Promise { if (toolReapBlocker(id, lath.getMeta(id)?.params) !== null) return false; - const entry = registry.get(id)!; - const inputVersion = entry.inputVersion ?? 0; + const entry = registry.get(id); const state = getTerminalPaneState(id); - const runId = state.currentCommand!.id; const cwd = state.currentCommand?.cwdAtStart?.path ?? state.cwd?.path ?? null; beginToolStop(id); let payload: string | null; @@ -110,18 +108,9 @@ export async function stopTool(lath: LathWallEngine, id: string): Promise