Keep unknown pane ownership legible through tolerant reads

Ordinary renderer polling could erase the very evidence the strict
column-agent launch depends on: tolerant recovery swept a pane whose
record it could not read out of the coordinator record, so a later
strict read found a clean set and could open a second review agent
beside a process nobody can account for. A pane whose record is present
but untrustworthy is now marked ownership-unknown and kept in the record
on both paths, while a pane that left no record at all is still swept as
the dead pane it is. The coordinator record gets the same treatment via
a strict read that separates ENOENT from corrupt. Failure copy selection
is now exhaustive over the reason codes, so a new recognised reason
cannot compile without its localized copy.

Author: h0x91b

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

@@ -12,6 +12,7 @@ Reproduced on a native task through the card's status menu: `launchColumnAgent S
 
 The pane is owned by purpose, not by a remembered id: `launchColumnAgent` goes through `openAuxPane` under a new `AuxPanePurpose` value `columnAgent` whose marker is the existing `col-agent.sh` temp path, the `col-agent-pane` id file is gone, and `replaceAuxPanes` closes every pane the purpose owns and re-reads the set to prove they went — a launch that cannot prove it refuses rather than risk two agents in one worktree.
 Proof covers the LOOKUP too, because several production paths turn an undecidable read into an empty list (`readPaneSet` catches every recovery exception, a `null` pane set becomes `[]`, an unreadable pane record becomes `command: []`, a tmux error becomes no rows), so the replacement path reads through `readPaneSetStrict` / `nativeTaskPaneCommandsStrict` and, at the root, through `recoverPaneSet(..., { strict: true })` — which throws `PaneOwnershipUnknownError` before reconciling an unknown-owner pane away, since sweeping it would delete the evidence while its shell keeps running.
+The line is drawn at whether there was anything to read: no record and no coordinator file mean the pane really is gone and it is swept exactly as before, while a record (or coordinator record) that is present and untrustworthy — corrupt, foreign-schema, unreadable — marks the pane ownership-unknown, and `recoverPaneSet` keeps that pane in the record even on the tolerant path, so ordinary renderer polling can no longer erase the evidence before a strict launch reads it.
 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?` and `reason?`, `columnAgentFailureCopy` picks one of four localized keys and localizes a built-in column from its status, and the report is emitted only after the fallback move's column write lands — a rejected or failed write stops the effect run, so the toast can never claim a move that did not happen, and the renderer never reads the English `error` string.
 

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

@@ -17,7 +17,7 @@
  * are asserted to stay untouched, which is the point of the strict path.
  */
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
-import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { createSplitTree, listPaneIds, serializeSplitTree, splitPane } from "../../shared/split-tree";
@@ -27,7 +27,7 @@ import {
 	writeRecordAtomic,
 	type NativeSessionRecord,
 } from "../native-terminal-registry/record";
-import { NATIVE_MULTIPANE_DIR_ENV } from "../native-terminal-multipane/paths";
+import { NATIVE_MULTIPANE_DIR_ENV, coordinatorRecordFile } from "../native-terminal-multipane/paths";
 import {
 	NATIVE_MULTIPANE_SCHEMA_VERSION,
 	readMultipaneRecord,
@@ -187,9 +187,10 @@ describe("strict native discovery, through the real coordinator", () => {
 		// the set, the coordinator record is rewritten, and stop() is told to drop it.
 		const tolerant = await nativePanes.nativeTaskPaneCommands(TASK_ID);
 		expect(tolerant.map((pane) => pane.sessionId)).not.toContain(reviewSession);
-		// Whatever tolerant recovery decides to do with it, the pane is invisible to
-		// the caller — which is precisely why the strict path may not reuse that answer.
-		expect(tolerant.map((pane) => pane.sessionId)).not.toContain(reviewSession);
+		// Whatever tolerant recovery decides to show, the pane's own record must stay
+		// on disk: erasing it is what would let a later strict read answer "nothing
+		// here" for a process that is still running.
+		expect(readMultipaneRecord(COORD_ID)!.panes.map((pane) => pane.sessionId)).toContain(reviewSession);
 
 		// Rebuild the same situation and take the strict path instead.
 		vi.clearAllMocks();
@@ -209,23 +210,98 @@ describe("strict native discovery, through the real coordinator", () => {
 		expect(before.panes).toHaveLength(2);
 	});
 
-	it("refuses when a pane's record file is missing outright, which the tolerant read hides", async () => {
+	it("refuses on a record written by a schema it does not understand, too", async () => {
 		writeTwoPaneSet(["/bin/bash", auxPaneMarker(TASK_ID, "columnAgent")]);
 		const reviewSession = reviewPaneSessionId();
 		const snapshot = readMultipaneRecord(COORD_ID)!;
+		// Parses fine, describes a live host, and says nothing this version can act on:
+		// another installed version of the app may own that process.
+		writeFileSync(
+			join(sessionDir(reviewSession), "record.json"),
+			JSON.stringify({ schemaVersion: NATIVE_SESSION_SCHEMA_VERSION + 7, sessionId: reviewSession }),
+		);
+
+		await expect(findAuxPanes(nativeTask, "columnAgent", SOCKET, { strict: true })).rejects.toBeInstanceOf(
+			PaneOwnershipUnknownError,
+		);
+		expect(mocks.registryStart).not.toHaveBeenCalled();
+		expect(mocks.registryStop).not.toHaveBeenCalled();
+		expect(readMultipaneRecord(COORD_ID)).toEqual(snapshot);
+	});
+
+	// Where the line is drawn, and why it is not drawn at "the record is unreadable
+	// for any reason at all". A finished pane unlinks its record and then tries to
+	// remove its directory, which fails whenever a sibling file survives — so "the
+	// directory is there but holds no record" is an ORDINARY dead pane. Calling that
+	// undecidable would wedge AI Review on every task that ever closed a pane.
+	it("sweeps a pane that left its directory behind without a record", async () => {
+		writeTwoPaneSet(["/bin/bash", auxPaneMarker(TASK_ID, "columnAgent")]);
+		const reviewSession = reviewPaneSessionId();
+		rmSync(join(sessionDir(reviewSession), "record.json"), { force: true });
+
+		await expect(findAuxPanes(nativeTask, "columnAgent", SOCKET, { strict: true })).resolves.toEqual([]);
+		expect(readMultipaneRecord(COORD_ID)!.panes.map((pane) => pane.sessionId)).not.toContain(reviewSession);
+	});
+
+	it("sweeps a pane whose session directory is gone entirely", async () => {
+		writeTwoPaneSet(["/bin/bash", auxPaneMarker(TASK_ID, "columnAgent")]);
+		const reviewSession = reviewPaneSessionId();
 		rmSync(sessionDir(reviewSession), { recursive: true, force: true });
 
-		// Strict first, so the assertions below describe an untouched set.
-		await expect(findAuxPanes(nativeTask, "columnAgent", SOCKET, { strict: true }))
-			.rejects.toBeInstanceOf(PaneOwnershipUnknownError);
+		await expect(findAuxPanes(nativeTask, "columnAgent", SOCKET, { strict: true })).resolves.toEqual([]);
+		expect(readMultipaneRecord(COORD_ID)!.panes.map((pane) => pane.sessionId)).not.toContain(reviewSession);
+	});
+
+	// THE REAL SEQUENCE. TaskTerminal polls taskPaneState continuously, so by the time
+	// the user clicks AI Review the tolerant path has already run several times. If a
+	// poll had swept the unknown-owner pane out of the coordinator record, the strict
+	// read that follows would find a clean, empty set and open a second agent beside a
+	// process nobody can account for.
+	it("still refuses after renderer-style tolerant polling has already run", async () => {
+		writeTwoPaneSet(["/bin/bash", auxPaneMarker(TASK_ID, "columnAgent")]);
+		const reviewSession = reviewPaneSessionId();
+		const snapshot = readMultipaneRecord(COORD_ID)!;
+		writeFileSync(join(sessionDir(reviewSession), "record.json"), "{ not json");
+
+		// Three polls, exactly as the terminal view would issue them.
+		for (let poll = 0; poll < 3; poll++) await nativePanes.nativeTaskPanesState(TASK_ID);
+
+		// The evidence survived every one of them.
+		expect(readMultipaneRecord(COORD_ID)).toEqual(snapshot);
+		expect(mocks.registryStop).not.toHaveBeenCalled();
+
+		// And the click that follows is refused, with nothing started.
+		await expect(openAuxPane(columnSpec())).rejects.toBeInstanceOf(AuxPaneUndecidableError);
 		expect(mocks.registryStart).not.toHaveBeenCalled();
 		expect(mocks.registryStop).not.toHaveBeenCalled();
 		expect(readMultipaneRecord(COORD_ID)).toEqual(snapshot);
+	});
 
-		// The tolerant read, by contrast, cannot see the pane at all — it is the answer
-		// the proven-replacement path must not reuse.
-		const tolerant = await nativePanes.nativeTaskPaneCommands(TASK_ID);
-		expect(tolerant.map((pane) => pane.sessionId)).not.toContain(reviewSession);
+	it("hides the unidentifiable pane from a tolerant read without deleting it", async () => {
+		writeTwoPaneSet(["/bin/bash", auxPaneMarker(TASK_ID, "columnAgent")]);
+		const reviewSession = reviewPaneSessionId();
+		writeFileSync(join(sessionDir(reviewSession), "record.json"), "{ not json");
+
+		const state = await nativePanes.nativeTaskPanesState(TASK_ID);
+
+		// The UI does not show a pane it cannot describe...
+		expect(state!.panes.map((pane) => pane.sessionId)).not.toContain(reviewSession);
+		// ...but the record still says the task owns it.
+		expect(readMultipaneRecord(COORD_ID)!.panes.map((pane) => pane.sessionId)).toContain(reviewSession);
+	});
+
+	it("refuses when the COORDINATOR record itself is corrupt, and changes nothing", async () => {
+		writeTwoPaneSet(["/bin/bash", auxPaneMarker(TASK_ID, "columnAgent")]);
+		const corrupt = "{ not a coordinator record";
+		writeFileSync(coordinatorRecordFile(COORD_ID), corrupt);
+
+		// The tolerant read cannot tell this from "no pane set at all".
+		await expect(nativePanes.nativeTaskPanesState(TASK_ID)).resolves.toBeNull();
+		// The strict one refuses instead of inventing an empty set.
+		await expect(openAuxPane(columnSpec())).rejects.toBeInstanceOf(AuxPaneUndecidableError);
+		expect(mocks.registryStart).not.toHaveBeenCalled();
+		expect(mocks.registryStop).not.toHaveBeenCalled();
+		expect(readFileSync(coordinatorRecordFile(COORD_ID), "utf8")).toBe(corrupt);
 	});
 
 	it("treats a genuinely absent pane set as owning nothing, not as undecidable", async () => {
@@ -234,9 +310,9 @@ describe("strict native discovery, through the real coordinator", () => {
 
 	it("keeps tolerant recovery tolerant for the best-effort purposes", async () => {
 		writeTwoPaneSet(["/bin/bash", auxPaneMarker(TASK_ID, "devServer")]);
-		rmSync(sessionDir(reviewPaneSessionId()), { recursive: true, force: true });
+		writeFileSync(join(sessionDir(reviewPaneSessionId()), "record.json"), "{ not json");
 
-		// devServer deliberately still reads a swept pane as "not there".
+		// devServer deliberately still reads an undecidable pane as "not there".
 		await expect(findAuxPanes(nativeTask, "devServer", SOCKET)).resolves.toEqual([]);
 	});
 });

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

@@ -22,6 +22,9 @@ vi.mock("../spawn", () => ({ spawn: vi.fn(), spawnSync: vi.fn() }));
 vi.mock("../native-terminal-registry/record", () => ({
 	readRecord: vi.fn(() => null),
 	readToken: vi.fn(() => null),
+	// Neither a record nor a session directory: a pane that is genuinely gone, which
+	// is what every case here means by "the host died".
+	inspectRecordFile: vi.fn(() => ({ ok: false, problem: { kind: "absent" } })),
 }));
 
 vi.mock("../native-terminal-registry/registry", () => ({

src/bun/native-terminal-multipane/coordinator.ts

@@ -22,7 +22,13 @@ import { withFileLock } from "../file-lock";
 import { NativeSessionClient } from "../native-terminal-registry/client";
 import { classifyOwnership, type OwnershipVerdict } from "../native-terminal-registry/ownership";
 import type { ClientRole } from "../native-terminal-registry/writer-ownership";
-import { readRecord, readToken, type NativeSessionRecord } from "../native-terminal-registry/record";
+import {
+	inspectRecordFile,
+	readRecord,
+	readToken,
+	type NativeSessionRecord,
+	type RecordInspection,
+} from "../native-terminal-registry/record";
 import { start, stop, type StartOptions, type StartResult } from "../native-terminal-registry/registry";
 import type { ShellLaunchSpec } from "../native-terminal-registry/shell-launch";
 import { CoordinatorClientView } from "./client-view";
@@ -42,6 +48,7 @@ import {
 	NATIVE_MULTIPANE_SCHEMA_VERSION,
 	pruneCoordinatorDir,
 	readMultipaneRecord,
+	readMultipaneRecordStrict,
 	removeMultipaneRecord,
 	writeMultipaneRecordAtomic,
 	type MultipanePaneEntry,
@@ -103,6 +110,12 @@ export interface CoordinatorDeps {
 	startPane(sessionId: string, opts: StartOptions): Promise<StartResult>;
 	stopPane(sessionId: string, opts?: { timeoutMs?: number }): Promise<boolean>;
 	readPaneRecord(sessionId: string): NativeSessionRecord | null;
+	/**
+	 * Why a record was rejected, for the one decision that needs it: telling a
+	 * record that is gone from one that is there but unreadable. Optional so an
+	 * in-memory double need not model file problems.
+	 */
+	inspectPaneRecord?(sessionId: string): RecordInspection;
 	readPaneToken(sessionId: string): string | null;
 	classifyPane(record: NativeSessionRecord, token: string | null): Promise<OwnershipVerdict>;
 	connectPane(record: NativeSessionRecord, token: string): Promise<PaneConnection>;
@@ -112,6 +125,7 @@ export const defaultCoordinatorDeps: CoordinatorDeps = {
 	startPane: (sessionId, opts) => start(sessionId, opts),
 	stopPane: (sessionId, opts) => stop(sessionId, opts ?? {}),
 	readPaneRecord: readRecord,
+	inspectPaneRecord: inspectRecordFile,
 	readPaneToken: readToken,
 	classifyPane: classifyOwnership,
 	async connectPane(record, token) {
@@ -231,7 +245,11 @@ export class NativeMultipaneCoordinator {
 		return withFileLock(
 			coordinatorRecordFile(coordinatorId),
 			async () => {
-				const record = readMultipaneRecord(coordinatorId);
+				// A strict caller must not have "no record" invented for it: a coordinator
+				// file that exists but cannot be parsed means the pane set is unknown.
+				const record = opts.strict === true
+					? readMultipaneRecordStrict(coordinatorId)
+					: readMultipaneRecord(coordinatorId);
 				if (!record) return null;
 				const tree = restoreSplitTree(record.layout);
 				if (!tree) return null;
@@ -240,21 +258,25 @@ export class NativeMultipaneCoordinator {
 				// to `ps`, so doing them in sequence made recovery cost grow linearly with
 				// the pane count on the click path (seq 1382).
 				const probes = await Promise.all(record.panes.map((pane) => probePane(pane, deps)));
-				// A strict caller is about to decide whether this task already owns a
-				// pane. Reconciling an unknown-owner pane away first would answer that
-				// question by deleting the evidence: the pane leaves the set, its shell
-				// keeps running (stop() reports success for a record it cannot read), and
-				// the caller opens a second agent beside it. So refuse BEFORE mutating
-				// anything — no drop, no record rewrite, no membership change.
+				// A pane whose own record we cannot read is not proof of death: its shell
+				// may still be running, and `stop()` reports success for a record it
+				// cannot read without ever signalling the pid. So the EVIDENCE that such
+				// a pane exists must outlive every read, tolerant ones included — a
+				// renderer poll that swept it out of the record would answer a later
+				// strict question by having deleted the answer.
 				const unknown = probes.filter((probe) => probe.ownershipUnknown);
 				if (opts.strict === true && unknown.length > 0) {
+					// Refuse BEFORE mutating anything: no drop, no rewrite, no membership
+					// change, so the caller sees the set exactly as it was found.
 					throw new PaneOwnershipUnknownError(
 						coordinatorId,
 						unknown.map((probe) => probe.pane.paneId),
 					);
 				}
-				const dead = probes.filter((probe) => probe.verdict !== "owned");
-				if (dead.length === 0) {
+				// Only a pane whose death was actually established may be reconciled away.
+				const dead = probes.filter((probe) => probe.verdict !== "owned" && !probe.ownershipUnknown);
+				const hidden = unknown.length > 0;
+				if (dead.length === 0 && !hidden) {
 					return {
 						coordinator: new NativeMultipaneCoordinator(coordinatorId, record.epoch, tree, deps),
 						panes: probes.map(snapshotOf),
@@ -269,11 +291,17 @@ export class NativeMultipaneCoordinator {
 				for (const probe of dead) reconciled = closeTreePane(reconciled, probe.pane.paneId);
 				reconciled = normalizeSharedLayout(reconciled);
 				await dropPanes(dead.map((probe) => probe.pane), deps);
-				writeMultipaneRecordAtomic(
-					buildRecord(coordinatorId, record.epoch, reconciled, bindPanes(coordinatorId, reconciled)),
-				);
+				// An unknown-owner pane stays in the record and in the tree; it is only
+				// HIDDEN from this snapshot, so the UI does not show a pane it cannot
+				// describe while the evidence stays on disk for the next strict read.
+				if (dead.length > 0) {
+					writeMultipaneRecordAtomic(
+						buildRecord(coordinatorId, record.epoch, reconciled, bindPanes(coordinatorId, reconciled)),
+					);
+				}
+				const survivorTree = hidden ? tree : reconciled;
 				return {
-					coordinator: new NativeMultipaneCoordinator(coordinatorId, record.epoch, reconciled, deps),
+					coordinator: new NativeMultipaneCoordinator(coordinatorId, record.epoch, survivorTree, deps),
 					// Survivors keep the record's order, which is the reconciled tree's order.
 					panes: probes.filter((probe) => probe.verdict === "owned").map(snapshotOf),
 				};
@@ -518,11 +546,14 @@ interface PaneProbe {
 
 async function probePane(pane: MultipanePaneEntry, deps: CoordinatorDeps): Promise<PaneProbe> {
 	const record = deps.readPaneRecord(pane.sessionId);
-	// A record we cannot read is NOT proof of death — the shell may well still be
-	// running, we simply lost the only thing that identifies it. Tolerant recovery
-	// keeps treating it as dead (that is how a crashed pane is swept), but the probe
-	// records the difference so a strict caller can refuse to guess.
-	if (!record) return { pane, record: null, verdict: "dead", ownershipUnknown: true };
+	// A record that is GONE is proof enough: a stopped pane takes its record with it,
+	// so there is nothing left to verify against and the pane is swept as before.
+	// A record that is PRESENT but unreadable is a different animal — the shell it
+	// described may still be running and `stop()` would report success without ever
+	// signalling the pid, so its ownership is unknown rather than dead.
+	if (!record) {
+		return { pane, record: null, verdict: "dead", ownershipUnknown: paneRecordPresentButUnreadable(pane, deps) };
+	}
 	return {
 		pane,
 		record,
@@ -531,6 +562,26 @@ async function probePane(pane: MultipanePaneEntry, deps: CoordinatorDeps): Promi
 	};
 }
 
+/**
+ * Whether the pane's record file exists yet cannot be interpreted. Falls back to
+ * `false` when a caller supplies no inspector, which keeps every existing
+ * in-memory dep double behaving exactly as it did.
+ */
+function paneRecordPresentButUnreadable(pane: MultipanePaneEntry, deps: CoordinatorDeps): boolean {
+	const inspection = deps.inspectPaneRecord?.(pane.sessionId);
+	if (!inspection || inspection.ok) return false;
+	switch (inspection.problem.kind) {
+		// Both mean "there is no record to read". A finished pane unlinks its record and
+		// only then tries to remove its directory, which fails whenever a sibling file
+		// survives — so a record-less directory is an ordinary dead pane, not a doubt.
+		case "absent":
+		case "missing":
+			return false;
+		default:
+			return true;
+	}
+}
+
 function snapshotOf({ pane, record, verdict }: PaneProbe): PaneSnapshot {
 	const { paneId, sessionId } = pane;
 	if (!record) return { paneId, sessionId, hostPid: -1, shellPid: -1, cols: 0, rows: 0, state: "dead" };