Refuse a proven replacement when native pane discovery is undecidable

Author: h0x91b-wix

decisions/197-column-agent-pane-ownership-and-failure-reason.md

@@ -10,13 +10,13 @@ Reproduced on a native task through the card's status menu: `launchColumnAgent S
 
 ## Decision
 
-The pane is owned by purpose, not by a remembered id: `launchColumnAgent` (`src/bun/rpc-handlers/tmux-pty.ts`) goes through `openAuxPane` under a new `AuxPanePurpose` value `columnAgent` (`src/bun/task-aux-panes.ts`), and the `col-agent-pane` id file is gone. That purpose is marked `provenReplace`, so `replaceAuxPanes` closes **every** pane it owns and re-reads the pane set to prove they are gone — a launch that cannot prove it refuses rather than risk two agents in one worktree. A stopped task terminal is never resurrected (that would cut across decision 184's explicit wake); the task is parked in Your Review with an actionable message instead.
+The pane is owned by purpose, not by a remembered id: `launchColumnAgent` (`src/bun/rpc-handlers/tmux-pty.ts`) goes through `openAuxPane` under a new `AuxPanePurpose` value `columnAgent` (`src/bun/task-aux-panes.ts`), and the `col-agent-pane` id file is gone. That purpose is marked `provenReplace`, so `replaceAuxPanes` closes **every** pane it owns and re-reads the pane set to prove they are gone — a launch that cannot prove it refuses rather than risk two agents in one worktree. Proof includes the LOOKUP, not just the close: both backends have production paths that turn an undecidable read into an empty list (`readPaneSet` catches every recovery exception, a `null` pane set becomes `[]`, an unreadable per-pane record becomes `command: []`, a tmux error becomes no rows), so the replacement path reads through `readPaneSetStrict` / `nativeTaskPaneCommandsStrict` and refuses via `AuxPaneUndecidableError` unless the empty list was actually observed. A stopped task terminal is never resurrected (that would cut across decision 184's explicit wake); the task is parked in Your Review with an actionable message instead.
 
 The failure is reported as well as parked. `columnAgentFailed` carries `column: ColumnAgentIdentity`, `movedTo?: TaskStatus` and `reason?: ColumnAgentFailureReason` (`src/shared/types.ts`); `columnAgentFailureCopy` (`src/mainview/utils/columnAgentFailureToast.ts`) picks one of four localized keys from `reason` × `movedTo` and localizes a built-in column's name from its status. The report rides on the fallback move and is emitted *after* that move's column write, because a rejected or failed write stops the effect run — a toast claiming a move that never landed is worse than silence. The renderer never reads the English `error` string; that stays diagnostics for failures the app cannot explain.
 
 ## Risks
 
-Each new recognised failure costs another reason value plus two keys × three locales — deliberate, because the alternatives are gluing localized fragments or matching on English. `auxPaneTitle("columnAgent")` is the generic "Column Agent" in the native pane picker, since the label is derived from the launch command and cannot know which column launched it; the pane's own OSC title is still the real column name.
+Each new recognised failure costs another reason value plus two keys × three locales — deliberate, because the alternatives are gluing localized fragments or matching on English. The strict read costs an extra ownership sweep on each replacement, and it refuses in cases the tolerant read would have sailed through — that is the point, but it does mean a flaky pane set now blocks AI Review instead of silently double-launching. `auxPaneTitle("columnAgent")` is the generic "Column Agent" in the native pane picker, since the label is derived from the launch command and cannot know which column launched it; the pane's own OSC title is still the real column name.
 
 ## Alternatives considered
 

src/bun/__tests__/ai-review-move-entry.test.ts

@@ -26,6 +26,10 @@ const mocks = vi.hoisted(() => ({
 	// native pane runtime
 	nativeTaskPanesState: vi.fn(),
 	nativeTaskPaneCommands: vi.fn(async () => [] as any[]),
+	// Serves the same fake pane list as the tolerant read; the undecidable and
+	// unreadable-record cases run against the real discovery path in
+	// column-agent-strict-discovery.test.ts.
+	nativeTaskPaneCommandsStrict: vi.fn(async () => ({ kind: "read", panes: await mocks.nativeTaskPaneCommands(), unreadable: [] as string[] })),
 	nativeTaskPaneCommandsOf: vi.fn(() => [] as any[]),
 	splitNativeTaskPane: vi.fn(),
 	closeNativeTaskPane: vi.fn(),
@@ -99,6 +103,7 @@ vi.mock("../artifact-template", () => ({ ensureArtifactTemplateEnv: () => ({}) }
 vi.mock("../native-task-panes", () => ({
 	nativeTaskPanesState: mocks.nativeTaskPanesState,
 	nativeTaskPaneCommands: mocks.nativeTaskPaneCommands,
+	nativeTaskPaneCommandsStrict: mocks.nativeTaskPaneCommandsStrict,
 	nativeTaskPaneCommandsOf: mocks.nativeTaskPaneCommandsOf,
 	splitNativeTaskPane: mocks.splitNativeTaskPane,
 	closeNativeTaskPane: mocks.closeNativeTaskPane,

src/bun/__tests__/column-agent-pane-recovery.test.ts

@@ -76,8 +76,16 @@ vi.mock("../native-task-panes", async (importOriginal) => {
 	return {
 		...actual,
 		nativeTaskPanesState: vi.fn(async () => state()),
-		// REAL: reads each pane's launch command back from its on-disk record.
+		// REAL: both reads take each pane's launch command back off its on-disk record.
 		nativeTaskPaneCommands: vi.fn(async () => actual.nativeTaskPaneCommandsOf(state())),
+		nativeTaskPaneCommandsStrict: vi.fn(async () => {
+			const panes = actual.nativeTaskPaneCommandsOf(state());
+			return {
+				kind: "read" as const,
+				panes,
+				unreadable: panes.filter((pane) => pane.command.length === 0).map((pane) => pane.paneId),
+			};
+		}),
 		splitNativeTaskPane: vi.fn(async () => ({ paneId: "pane-new", state: state() })),
 		// A real close: the record leaves the disk, so the verification re-read has
 		// something honest to look at.

src/bun/__tests__/column-agent-strict-discovery.test.ts

@@ -0,0 +1,215 @@
+/**
+ * "I could not tell" must never read as "there is nothing there" — over the REAL
+ * native discovery path.
+ *
+ * Three production behaviours each collapse an undecidable read into an empty
+ * list, and any of them would let a second review agent open beside a live one:
+ *
+ *  • `NativeTerminalBackend.readPaneSet` catches every recovery exception and
+ *    returns `null`;
+ *  • `nativeTaskPaneCommands` turns a `null` pane set into `[]`;
+ *  • `nativeTaskPaneCommandsOf` turns a pane whose own record is unreadable into
+ *    `command: []`, so it matches no marker.
+ *
+ * So nothing here mocks the module under test. The registry directory is real, the
+ * records are real files, and the failures are injected where they actually happen:
+ * in `recoverPaneSet` and in the per-pane record on disk.
+ */
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { NATIVE_SESSIONS_DIR_ENV, recordFile, sessionDir } from "../native-terminal-registry/paths";
+import {
+	NATIVE_SESSION_SCHEMA_VERSION,
+	writeRecordAtomic,
+	type NativeSessionRecord,
+} from "../native-terminal-registry/record";
+import type { Task } from "../../shared/types";
+
+const TASK_ID = "dddddddd-0000-0000-0000-000000000004";
+const SOCKET = "dev3-sock";
+const nativeTask = { id: TASK_ID, seq: 909, terminalBackend: "native" } as unknown as Task;
+
+/** What the coordinator's recovery does when asked for this task's pane set. */
+let recovery: () => Promise<{ panes: { paneId: string; sessionId: string }[] } | null>;
+
+vi.mock("../logger", () => ({
+	createLogger: () => ({ debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }),
+}));
+vi.mock("../spawn", () => ({ spawn: vi.fn(), spawnSync: vi.fn() }));
+vi.mock("../tmux", () => ({
+	PANE_START_COMMAND_FORMAT: { formatString: "", parse: () => [] },
+	TmuxError: class extends Error {},
+	taskSessionName: (taskId: string) => `dev3-${taskId.slice(0, 8)}`,
+	tmux: {
+		listPanes: vi.fn(() => {
+			throw new Error("a native task must not reach tmux");
+		}),
+		splitWindow: vi.fn(() => {
+			throw new Error("a native task must not reach tmux");
+		}),
+		selectPane: vi.fn(),
+		killPane: vi.fn(() => {
+			throw new Error("a native task must not reach tmux");
+		}),
+	},
+}));
+
+// The ONE injection point: the coordinator's own recovery. Everything above it —
+// readPaneSet's catch, buildState's null handling, the per-pane record read — is
+// the real production code.
+vi.mock("../native-terminal-multipane/coordinator", async (importOriginal) => {
+	const actual = await importOriginal<typeof import("../native-terminal-multipane/coordinator")>();
+	return {
+		...actual,
+		NativeMultipaneCoordinator: class {
+			static async recoverPaneSet() {
+				const recovered = await recovery();
+				if (!recovered) return null;
+				return {
+					coordinator: { layout: { activePaneId: recovered.panes[0]?.paneId ?? "", root: null } },
+					panes: recovered.panes.map(({ paneId, sessionId }) => ({
+						paneId,
+						sessionId,
+						hostPid: 1,
+						shellPid: 2,
+						cols: 80,
+						rows: 24,
+						state: "alive",
+					})),
+				};
+			}
+		},
+	};
+});
+vi.mock("../../shared/split-tree", async (importOriginal) => ({
+	...(await importOriginal<typeof import("../../shared/split-tree")>()),
+	serializeSplitTree: () => "layout",
+}));
+
+const {
+	auxPaneMarker,
+	findAuxPanes,
+	openAuxPane,
+	AuxPaneUndecidableError,
+} = await import("../task-aux-panes");
+const { splitNativeTaskPane } = await import("../native-task-panes");
+const nativePanes = await import("../native-task-panes");
+
+let sessionsDir: string;
+
+function record(sessionId: string, paneId: string, command: string[]): NativeSessionRecord {
+	return {
+		schemaVersion: NATIVE_SESSION_SCHEMA_VERSION,
+		sessionId,
+		paneId,
+		protocolVersion: 1,
+		hostArtifactVersion: "1",
+		runtimeVersion: "1.3.14",
+		platform: "darwin",
+		host: { pid: 1, executable: "/bin/bun", startSignature: "1@t0" },
+		shell: { pid: 2, command, startSignature: "2@t0" },
+		endpoint: { transport: "ws", address: "127.0.0.1", port: 51234 },
+		ownership: { evidenceKind: "posix-start-signature" },
+		cols: 80,
+		rows: 24,
+		createdAt: "2026-08-02T00:00:00.000Z",
+		updatedAt: "2026-08-02T00:00:00.000Z",
+	};
+}
+
+function columnSpec() {
+	const marker = auxPaneMarker(TASK_ID, "columnAgent");
+	return {
+		task: nativeTask,
+		purpose: "columnAgent" as const,
+		placement: "right" as const,
+		size: "40%",
+		cwd: "/tmp/wt",
+		socket: SOCKET,
+		title: "AI Review",
+		tmuxCommand: `bash "${marker}"`,
+		nativeLaunch: { executable: "/bin/bash", argv: [marker] },
+	};
+}
+
+beforeEach(() => {
+	vi.clearAllMocks();
+	sessionsDir = mkdtempSync(join(tmpdir(), "dev3-strict-discovery-"));
+	process.env[NATIVE_SESSIONS_DIR_ENV] = sessionsDir;
+	nativePanes._resetBackendForTests();
+	recovery = async () => null;
+});
+
+afterEach(() => {
+	delete process.env[NATIVE_SESSIONS_DIR_ENV];
+	rmSync(sessionsDir, { recursive: true, force: true });
+});
+
+describe("strict native discovery for a proven replacement", () => {
+	it("refuses when recovery throws, instead of reading it as an empty pane set", async () => {
+		recovery = async () => {
+			throw new Error("ownership sweep failed");
+		};
+
+		// The tolerant read is what production does elsewhere, and it hides this.
+		await expect(nativePanes.nativeTaskPaneCommands(TASK_ID)).resolves.toEqual([]);
+		// The replacement path must not accept that answer.
+		await expect(findAuxPanes(nativeTask, "columnAgent", SOCKET, { strict: true })).rejects.toThrow(/ownership sweep/);
+		await expect(openAuxPane(columnSpec())).rejects.toBeInstanceOf(AuxPaneUndecidableError);
+		expect(splitNativeTaskPane).toBeDefined();
+	});
+
+	it("refuses when a pane's own launch command cannot be read", async () => {
+		const marker = auxPaneMarker(TASK_ID, "columnAgent");
+		writeRecordAtomic(record("sess-agent", "pane-1", ["/bin/zsh"]));
+		writeRecordAtomic(record("sess-review", "pane-9", ["/bin/bash", marker]));
+		// A record that exists but cannot be parsed — the case that silently becomes
+		// `command: []` and therefore matches no marker.
+		writeFileSync(join(sessionDir("sess-review"), "record.json"), "{ not json");
+		recovery = async () => ({
+			panes: [
+				{ paneId: "pane-1", sessionId: "sess-agent" },
+				{ paneId: "pane-9", sessionId: "sess-review" },
+			],
+		});
+
+		// Tolerant read: the review pane looks like it is not there at all.
+		const tolerant = await nativePanes.nativeTaskPaneCommands(TASK_ID);
+		expect(tolerant.find((pane) => pane.paneId === "pane-9")?.command).toEqual([]);
+		// Strict read refuses rather than opening a second agent beside it.
+		await expect(openAuxPane(columnSpec())).rejects.toBeInstanceOf(AuxPaneUndecidableError);
+	});
+
+	it("treats a genuinely absent pane set as owning nothing, not as undecidable", async () => {
+		recovery = async () => null;
+
+		await expect(findAuxPanes(nativeTask, "columnAgent", SOCKET, { strict: true })).resolves.toEqual([]);
+	});
+
+	it("keeps the tolerant read tolerant for the best-effort purposes", async () => {
+		recovery = async () => {
+			throw new Error("ownership sweep failed");
+		};
+
+		await expect(findAuxPanes(nativeTask, "devServer", SOCKET)).resolves.toEqual([]);
+	});
+
+	it("reads a healthy pane set through the real record files", async () => {
+		const marker = auxPaneMarker(TASK_ID, "columnAgent");
+		writeRecordAtomic(record("sess-agent", "pane-1", ["/bin/zsh"]));
+		writeRecordAtomic(record("sess-review", "pane-9", ["/bin/bash", marker]));
+		recovery = async () => ({
+			panes: [
+				{ paneId: "pane-1", sessionId: "sess-agent" },
+				{ paneId: "pane-9", sessionId: "sess-review" },
+			],
+		});
+
+		await expect(findAuxPanes(nativeTask, "columnAgent", SOCKET, { strict: true })).resolves.toEqual([
+			{ backend: "native", paneId: "pane-9" },
+		]);
+		expect(recordFile("sess-review")).toContain("sess-review");
+	});
+});

src/bun/__tests__/rpc-handlers.test.ts

@@ -159,6 +159,10 @@ const mockNativePanes = {
 	nativeTaskPaneLayout: vi.fn(async (..._args: any[]) => null as any),
 	nativeTaskPanesAlive: vi.fn(async () => false),
 	nativeTaskPaneCommands: vi.fn(async () => [] as any[]),
+	// Serves the same fake pane list as the tolerant read; the undecidable and
+	// unreadable-record cases run against the real discovery path in
+	// column-agent-strict-discovery.test.ts.
+	nativeTaskPaneCommandsStrict: vi.fn(async () => ({ kind: "read", panes: await mockNativePanes.nativeTaskPaneCommands(), unreadable: [] as string[] })),
 	nativeTaskPaneCommandsOf: vi.fn(() => [] as any[]),
 	splitNativeTaskPane: vi.fn(async (..._args: any[]) => null as any),
 	closeNativeTaskPane: vi.fn(async (..._args: any[]) => ({ sessionTornDown: false, state: null }) as any),

src/bun/__tests__/task-aux-panes.test.ts

@@ -26,6 +26,7 @@ const mocks = vi.hoisted(() => ({
 	// native-task-panes
 	nativeTaskPanesState: vi.fn(),
 	nativeTaskPaneCommands: vi.fn(),
+	nativeTaskPaneCommandsStrict: vi.fn(),
 	splitNativeTaskPane: vi.fn(),
 	closeNativeTaskPane: vi.fn(),
 	focusNativeTaskPane: vi.fn(),
@@ -56,6 +57,10 @@ vi.mock("../tmux", () => ({
 vi.mock("../native-task-panes", () => ({
 	nativeTaskPanesState: mocks.nativeTaskPanesState,
 	nativeTaskPaneCommands: mocks.nativeTaskPaneCommands,
+	// The strict read answers from the same fake pane list; the cases that make it
+	// throw or report an unreadable record live in column-agent-strict-discovery,
+	// where the production discovery path runs for real.
+	nativeTaskPaneCommandsStrict: mocks.nativeTaskPaneCommandsStrict,
 	splitNativeTaskPane: mocks.splitNativeTaskPane,
 	closeNativeTaskPane: mocks.closeNativeTaskPane,
 	focusNativeTaskPane: mocks.focusNativeTaskPane,
@@ -128,6 +133,11 @@ beforeEach(() => {
 	vi.clearAllMocks();
 	mocks.nativeTaskPanesState.mockResolvedValue(nativeState(["pane-1"], "pane-1"));
 	mocks.nativeTaskPaneCommands.mockResolvedValue([]);
+	mocks.nativeTaskPaneCommandsStrict.mockImplementation(async () => ({
+		kind: "read",
+		panes: await mocks.nativeTaskPaneCommands(),
+		unreadable: [],
+	}));
 	mocks.splitNativeTaskPane.mockResolvedValue({ paneId: "pane-2", state: nativeState(["pane-1", "pane-2"], "pane-2") });
 	mocks.closeNativeTaskPane.mockResolvedValue({ sessionTornDown: false, state: nativeState(["pane-1"], "pane-1") });
 	mocks.focusNativeTaskPane.mockResolvedValue(nativeState(["pane-1", "pane-2"], "pane-1"));

src/bun/native-task-panes.ts

@@ -96,7 +96,21 @@ function coordinatorId(taskId: string): string {
  * `null` means no pane survived — the same verdict `describeSession` returns.
  */
 async function buildState(taskId: string): Promise<NativeTaskPanesState | null> {
-	const paneSet = await getBackend().readPaneSet(coordinatorId(taskId));
+	return shapeState(taskId, await getBackend().readPaneSet(coordinatorId(taskId)));
+}
+
+/**
+ * {@link buildState} that does not hide an undecidable read: `null` means recovery
+ * ran and no pane survived, while a throw means the pane set could not be read.
+ */
+async function buildStateStrict(taskId: string): Promise<NativeTaskPanesState | null> {
+	return shapeState(taskId, await getBackend().readPaneSetStrict(coordinatorId(taskId)));
+}
+
+/** Derived from the backend method so this module keeps its single import seam. */
+type ReadPaneSet = Awaited<ReturnType<NativeTerminalBackend["readPaneSet"]>>;
+
+function shapeState(taskId: string, paneSet: ReadPaneSet): NativeTaskPanesState | null {
 	if (!paneSet) return null;
 	return {
 		taskId,
@@ -343,6 +357,40 @@ export async function nativeTaskPaneCommands(taskId: string): Promise<NativeTask
 	return state ? nativeTaskPaneCommandsOf(state) : [];
 }
 
+/**
+ * What a caller learns when "I could not tell" must not be reported as "there is
+ * nothing". `absent` is a real answer — recovery ran and this task owns no pane
+ * set. Anything it could not determine throws instead, and a pane whose own record
+ * is unreadable is listed in `unreadable`: its launch command is unknown, so no
+ * caller may claim the pane is not theirs.
+ */
+export type NativeTaskPaneCommandsRead =
+	| { kind: "absent" }
+	| { kind: "read"; panes: NativeTaskPaneCommand[]; unreadable: string[] };
+
+export async function nativeTaskPaneCommandsStrict(taskId: string): Promise<NativeTaskPaneCommandsRead> {
+	const state = await buildStateStrict(taskId);
+	if (!state) return { kind: "absent" };
+	const panes = state.panes.map((pane) => {
+		const record = readRecord(pane.sessionId);
+		return {
+			pane: {
+				paneId: pane.paneId,
+				sessionId: pane.sessionId,
+				command: record?.shell.command ?? [],
+				shellPid: pane.shellPid,
+				alive: pane.alive,
+			},
+			readable: record !== null,
+		};
+	});
+	return {
+		kind: "read",
+		panes: panes.map((entry) => entry.pane),
+		unreadable: panes.filter((entry) => !entry.readable).map((entry) => entry.pane.paneId),
+	};
+}
+
 /**
  * True when the coordinator record exists and contains at least one owned pane.
  * Read-only: does NOT register or cache the recovered coordinator as a side effect.