fix: release pi review ports on session shutdown

apps/pi-extension/browser-session-error.ts

@@ -0,0 +1,13 @@
+/**
+ * Typed marker for the "browser session was stopped" outcome.
+ *
+ * Kept in a zero-import module so index.ts can classify the error
+ * synchronously without pulling in plannotator-browser.ts's heavy
+ * server/browser import graph.
+ */
+export const BROWSER_SESSION_STOPPED = "PlannotatorBrowserSessionStopped";
+
+/** True when an error is the typed stopped-session outcome, not a real failure. */
+export function isBrowserSessionStoppedError(err: unknown): boolean {
+	return err instanceof Error && err.name === BROWSER_SESSION_STOPPED;
+}

apps/pi-extension/index.ts

@@ -73,6 +73,7 @@ import {
 	stripPlanningOnlyTools,
 } from "./tool-scope.ts";
 import { isRemoteSession } from "./server/network.ts";
+import { isBrowserSessionStoppedError } from "./browser-session-error.ts";
 import { classifyAnnotateOutcome } from "./annotate-outcome.ts";
 
 // ── Types ──────────────────────────────────────────────────────────────
@@ -164,6 +165,13 @@ function sessionOpenedMessage(label: string, url: string): string {
 function reportBackgroundError(ctx: ExtensionContext, message: string, err: unknown, origin?: PiSessionIdentity): void {
 	const detail = getStartupErrorMessage(err);
 	console.error(`${message}: ${detail}`);
+	// A stopped session is not a failure: it is how supersession ends a stale
+	// undecided session (port self-preemption, #1159) and how cancel paths
+	// settle a pending waitForDecision.
+	if (isBrowserSessionStoppedError(err)) {
+		safeNotify(ctx, "A Plannotator browser session was closed.", "info", origin);
+		return;
+	}
 	safeNotify(ctx, `${message}: ${detail}`, "error", origin);
 }
 
@@ -286,6 +294,11 @@ export default function plannotator(pi: ExtensionAPI): void {
 	pi.on("session_shutdown", () => {
 		sessionAlive = false;
 		currentPiSession.clear();
+		// Browser sessions deliberately outlive in-process session replacement so
+		// a tab opened before /new can still deliver feedback to the replacement
+		// session (withCurrentPiSessionFallbackHeader). On real process teardown
+		// the OS frees the ports, and port self-preemption reclaims any stale
+		// fixed-port session on the next command.
 	});
 
 	// ── Flags ────────────────────────────────────────────────────────────
@@ -940,7 +953,7 @@ export default function plannotator(pi: ExtensionAPI): void {
 			}),
 		}) as any,
 
-		async execute(_toolCallId, params, _signal, _onUpdate, ctx) {
+		async execute(_toolCallId, params, signal, _onUpdate, ctx) {
 			// Guard: must be in planning phase
 			if (phase !== "planning") {
 				return {
@@ -1066,8 +1079,22 @@ export default function plannotator(pi: ExtensionAPI): void {
 
 			let result: Awaited<ReturnType<typeof openPlanReviewBrowser>>;
 			try {
-				result = await openPlanReviewBrowser(ctx, planContent);
+				result = await openPlanReviewBrowser(ctx, planContent, signal);
 			} catch (err) {
+				// A stopped session is an outcome, not a startup failure: the review
+				// was closed (cancellation or port self-preemption) before a decision.
+				if (isBrowserSessionStoppedError(err)) {
+					ctx.ui.notify("Plan review session was closed before a decision.", "info");
+					return {
+						content: [
+							{
+								type: "text",
+								text: "The plan review browser session was closed before a decision was made. The plan was neither approved nor rejected; resubmit to reopen review.",
+							},
+						],
+						details: { approved: false },
+					};
+				}
 				const message = `Failed to start plan review UI: ${getStartupErrorMessage(err)}`;
 				ctx.ui.notify(message, "error");
 				return {

apps/pi-extension/package.json

@@ -30,6 +30,7 @@
     "index.ts",
     "annotate-outcome.ts",
     "assistant-message.ts",
+    "browser-session-error.ts",
     "current-pi-session.ts",
     "plannotator-browser.ts",
     "plannotator-browser-runtime.ts",

apps/pi-extension/plannotator-browser.test.ts

@@ -1,5 +1,33 @@
-import { describe, expect, test } from "bun:test";
-import { shouldUseLocalPrCheckout } from "./plannotator-browser.ts";
+// Remote mode suppresses the browser launch so these tests never spawn the
+// developer's browser or read their ~/.plannotator config. BROWSER overrides
+// bypass the remote-mode suppression, so they are cleared too. (env reads
+// happen at call time, so setting these before the tests run is sufficient.)
+// Restored in afterAll: bun test shares one process across files, and other
+// suites assert non-remote behavior.
+const savedEnv = {
+	PLANNOTATOR_REMOTE: process.env.PLANNOTATOR_REMOTE,
+	PLANNOTATOR_BROWSER: process.env.PLANNOTATOR_BROWSER,
+	BROWSER: process.env.BROWSER,
+};
+process.env.PLANNOTATOR_REMOTE = "1";
+delete process.env.PLANNOTATOR_BROWSER;
+delete process.env.BROWSER;
+
+import { afterAll, describe, expect, test } from "bun:test";
+import {
+	getActiveBrowserSessionCount,
+	shouldUseLocalPrCheckout,
+	startBrowserDecisionSession,
+	startServerWithSelfPreemption,
+	stopAllBrowserDecisionSessions,
+} from "./plannotator-browser.ts";
+
+afterAll(() => {
+	for (const [key, value] of Object.entries(savedEnv)) {
+		if (value === undefined) delete process.env[key];
+		else process.env[key] = value;
+	}
+});
 
 describe("shouldUseLocalPrCheckout", () => {
 	test("uses local PR checkout by default", () => {
@@ -11,3 +39,159 @@ describe("shouldUseLocalPrCheckout", () => {
 		expect(shouldUseLocalPrCheckout({ useLocal: false })).toBe(false);
 	});
 });
+
+describe("browser session cleanup", () => {
+	const ctx = {
+		hasUI: true,
+		ui: {
+			notify() {},
+			theme: { fg: () => "" },
+			setStatus() {},
+		},
+	} as any;
+
+	test("positive: host shutdown stops every active browser session and rejects its pending decision", async () => {
+		const stopCounts = [0, 0];
+		const never = new Promise<never>(() => {});
+		const sessions = stopCounts.map((_, index) =>
+			startBrowserDecisionSession(
+				{ url: `http://localhost:${index + 1}`, stop: () => stopCounts[index]++ },
+				ctx,
+				() => never,
+			),
+		);
+		const decisions = sessions.map((session) => session.waitForDecision().catch((error) => error));
+
+		stopAllBrowserDecisionSessions();
+
+		expect(stopCounts).toEqual([1, 1]);
+		expect(getActiveBrowserSessionCount()).toBe(0);
+		for (const decision of decisions) {
+			const error = await decision;
+			expect(error).toBeInstanceOf(Error);
+			expect(error.message).toBe("Plannotator browser session was stopped.");
+		}
+	});
+
+	test("positive: tool cancellation stops its browser session immediately", async () => {
+		let stops = 0;
+		const controller = new AbortController();
+		const session = startBrowserDecisionSession(
+			{ url: "http://localhost:1", stop: () => stops++ },
+			ctx,
+			() => new Promise<never>(() => {}),
+			controller.signal,
+		);
+		const decision = session.waitForDecision().catch((error) => error);
+
+		controller.abort();
+
+		expect(stops).toBe(1);
+		expect((await decision).message).toBe("Plannotator browser session was stopped.");
+	});
+
+	test("negative: an already-aborted tool never leaves an active browser session", async () => {
+		let stops = 0;
+		const controller = new AbortController();
+		controller.abort();
+
+		const session = startBrowserDecisionSession(
+			{ url: "http://localhost:1", stop: () => stops++ },
+			ctx,
+			() => new Promise<never>(() => {}),
+			controller.signal,
+		);
+		const decision = session.waitForDecision().catch((error) => error);
+		stopAllBrowserDecisionSessions();
+
+		expect(stops).toBe(1);
+		expect((await decision).message).toBe("Plannotator browser session was stopped.");
+	});
+
+	test("negative: host shutdown does not stop an already-stopped session again", async () => {
+		let stops = 0;
+		const session = startBrowserDecisionSession(
+			{ url: "http://localhost:1", stop: () => stops++ },
+			ctx,
+			() => new Promise<never>(() => {}),
+		);
+		const decision = session.waitForDecision().catch((error) => error);
+
+		session.stop();
+		stopAllBrowserDecisionSessions();
+		stopAllBrowserDecisionSessions();
+
+		expect(stops).toBe(1);
+		expect((await decision).message).toBe("Plannotator browser session was stopped.");
+	});
+
+	test("negative: host shutdown is a no-op when no browser sessions are active", () => {
+		// Every prior test stopped its sessions, so the registry must be empty;
+		// a leaked entry here would make the no-op claim vacuous.
+		expect(getActiveBrowserSessionCount()).toBe(0);
+		expect(() => stopAllBrowserDecisionSessions()).not.toThrow();
+		expect(getActiveBrowserSessionCount()).toBe(0);
+	});
+
+	test("positive: self-preemption stops only sessions registered before the failing start", async () => {
+		let staleStops = 0;
+		let freshStops = 0;
+		const never = new Promise<never>(() => {});
+		startBrowserDecisionSession(
+			{ url: "http://localhost:1", stop: () => staleStops++ },
+			ctx,
+			() => never,
+		);
+		// Date.now() has millisecond resolution; make the stale session strictly
+		// older than the failing start.
+		await new Promise((resolve) => setTimeout(resolve, 5));
+
+		let attempts = 0;
+		const result = await startServerWithSelfPreemption(async () => {
+			attempts++;
+			if (attempts === 1) {
+				// A concurrent sibling command binds while this start is failing;
+				// its fresh session must survive the sweep.
+				startBrowserDecisionSession(
+					{ url: "http://localhost:2", stop: () => freshStops++ },
+					ctx,
+					() => never,
+				);
+				throw new Error("Port 19432 in use after 5 retries");
+			}
+			return "ok";
+		});
+
+		expect(result).toBe("ok");
+		expect(attempts).toBe(2);
+		expect(staleStops).toBe(1);
+		expect(freshStops).toBe(0);
+		expect(getActiveBrowserSessionCount()).toBe(1);
+		stopAllBrowserDecisionSessions();
+		expect(freshStops).toBe(1);
+		expect(getActiveBrowserSessionCount()).toBe(0);
+	});
+
+	test("negative: self-preemption rethrows when every tracked session is younger than the failing start", async () => {
+		let freshStops = 0;
+		const never = new Promise<never>(() => {});
+		let attempts = 0;
+		await expect(
+			startServerWithSelfPreemption(async () => {
+				attempts++;
+				startBrowserDecisionSession(
+					{ url: "http://localhost:1", stop: () => freshStops++ },
+					ctx,
+					() => never,
+				);
+				throw new Error("Port 19432 in use after 5 retries");
+			}),
+		).rejects.toThrow("Port 19432 in use after 5 retries");
+
+		expect(attempts).toBe(1);
+		expect(freshStops).toBe(0);
+		expect(getActiveBrowserSessionCount()).toBe(1);
+		stopAllBrowserDecisionSessions();
+		expect(getActiveBrowserSessionCount()).toBe(0);
+	});
+});