From a89923a1150058a29ae6490c125e30fe5f4706c8 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Tue, 28 Jul 2026 13:01:15 -0700 Subject: [PATCH 1/2] feat: add external presenter lifecycle --- README.md | 19 + apps/hook/server/index.ts | 133 +++--- apps/hook/server/server-shutdown.test.ts | 120 +++++ apps/hook/server/server-shutdown.ts | 75 +++ apps/opencode-plugin/commands.ts | 18 +- apps/pi-extension/index.ts | 29 +- .../plannotator-browser-runtime.ts | 25 + apps/pi-extension/plannotator-browser.test.ts | 431 +++++++++++++++++- apps/pi-extension/plannotator-browser.ts | 418 ++++++++++++----- apps/pi-extension/plannotator-events.ts | 12 + apps/pi-extension/vendor.sh | 2 +- apps/review/server/index.ts | 6 +- packages/server/annotate.ts | 53 ++- packages/server/goal-setup.ts | 45 +- packages/server/index.ts | 11 +- packages/server/port-startup-compat.test.ts | 194 ++++++++ packages/server/review.ts | 71 ++- packages/server/shared-handlers.test.ts | 46 ++ packages/server/shared-handlers.ts | 23 +- packages/shared/config.ts | 15 + packages/shared/package.json | 1 + packages/shared/presenter.test.ts | 293 ++++++++++++ packages/shared/presenter.ts | 396 ++++++++++++++++ .../test-fixtures/presenter-fixture.mjs | 34 ++ 24 files changed, 2233 insertions(+), 237 deletions(-) create mode 100644 apps/hook/server/server-shutdown.test.ts create mode 100644 apps/hook/server/server-shutdown.ts create mode 100644 packages/shared/presenter.test.ts create mode 100644 packages/shared/presenter.ts create mode 100755 packages/shared/test-fixtures/presenter-fixture.mjs diff --git a/README.md b/README.md index 504946fed..b7019caf8 100644 --- a/README.md +++ b/README.md @@ -363,6 +363,7 @@ implementation architecture. | `PLANNOTATOR_REMOTE` | `1`/`true` for remote mode, `0`/`false` for local, unset for SSH auto-detection | | `PLANNOTATOR_PORT` | Fixed port (default: random locally, `19432` remote) | | `PLANNOTATOR_BROWSER` | Custom browser to open plans in | +| `PLANNOTATOR_PRESENTER` | Executable implementing the one-request JSON presenter protocol; overrides `config.json` and may run outside Herdr | | `PLANNOTATOR_AI` | `disabled` to disable Ask AI, Review Agents, and Guided Review; the annotate agent terminal is separate | | `PLANNOTATOR_SHARE` | `disabled` to turn off URL sharing | | `PLANNOTATOR_SHARE_URL` | Custom base URL for share links (self-hosted portal) | @@ -378,6 +379,24 @@ All Plannotator data lives in a single directory — `~/.plannotator` by default export PLANNOTATOR_DATA_DIR=~/.local/share/plannotator ``` +Host integrations can persist an external presenter in +`~/.plannotator/config.json`: + +```json +{ + "presenter": { + "command": "/absolute/path/to/presenter", + "when": "herdr" + } +} +``` + +The command is executed directly without a shell. `when` defaults to +`"herdr"`, so the presenter is selected only when `HERDR_ENV=1`; use +`"always"` to enable it everywhere. An explicitly set +`PLANNOTATOR_PRESENTER` takes priority, and an empty value disables the +configured presenter for that invocation. + --- ## Development diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 7f1298e6b..d85490d0b 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -158,6 +158,7 @@ import { createAnnotateOutcomeEmitter, supportsAnnotateApprovalNotes, } from "./annotate-output"; +import { createServerShutdownCoordinator } from "./server-shutdown"; // Embed the built HTML at compile time // @ts-ignore - Bun import attribute for text @@ -286,16 +287,30 @@ if (isInteractiveNoArgInvocation(args, process.stdin.isTTY)) { process.exit(0); } -// Ensure session cleanup on exit +// Ensure session cleanup on every explicit exit, including both graceful and +// force-exit signal paths. process.on("exit", () => unregisterSession()); -// Route fatal signals through process.exit() so "exit" handlers run — by -// default a SIGINT/SIGTERM death skips them, leaking background-warmup -// children and stale `git worktree` registrations (the --local PR checkout -// cleanup below is registered on "exit"). `once` keeps a second Ctrl-C as a -// force-quit escape hatch if cleanup ever hangs. -process.once("SIGINT", () => process.exit(130)); -process.once("SIGTERM", () => process.exit(143)); +const serverShutdown = createServerShutdownCoordinator({ + exit: (code) => process.exit(code), + onStopError: (error) => { + console.error( + `[plannotator] Failed to stop server during shutdown: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + }, +}); + +// The first signal waits for the current server's idempotent stop routine, +// including external presenter dismissal. A second signal force-exits if +// cleanup hangs. process.exit() still runs the unregisterSession handler above. +process.on("SIGINT", () => { + void serverShutdown.handleSignal("SIGINT"); +}); +process.on("SIGTERM", () => { + void serverShutdown.handleSignal("SIGTERM"); +}); // Check if URL sharing is enabled (default: true) const sharingEnabled = resolveSharingEnabled(loadConfig()); @@ -495,14 +510,13 @@ if (args[0] === "sessions") { const goalProject = (await detectProjectName()) ?? "_unknown"; - const server = await startGoalSetupServer({ + const server = await serverShutdown.trackServerStart(startGoalSetupServer({ bundle, origin: detectedOrigin, htmlContent: planHtmlContent, - onReady: (url, isRemote, port) => { - handleGoalSetupServerReady(url, isRemote, port); - }, - }); + onReady: (url, isRemote, port) => + handleGoalSetupServerReady(url, isRemote, port), + })); registerSession({ pid: process.pid, @@ -516,7 +530,7 @@ if (args[0] === "sessions") { const result = await server.waitForDecision(); await Bun.sleep(800); - server.stop(); + await server.stop(); if (result.exit) { console.log(JSON.stringify({ decision: "dismissed", stage: bundle.stage })); @@ -833,7 +847,7 @@ if (args[0] === "sessions") { const reviewProject = (await detectProjectName()) ?? "_unknown"; // Start review server (even if empty - user can switch diff types in local mode) - const server = await startReviewServer({ + const server = await serverShutdown.trackServerStart(startReviewServer({ rawPatch, gitRef, error: diffError, @@ -851,13 +865,13 @@ if (args[0] === "sessions") { htmlContent: reviewHtmlContent, onCleanup: worktreeCleanup, onReady: async (url, isRemote, port) => { - handleReviewServerReady(url, isRemote, port); + await handleReviewServerReady(url, isRemote, port); if (isRemote && sharingEnabled && rawPatch) { await writeRemoteShareLink(rawPatch, shareBaseUrl, "review changes", "diff only").catch(() => {}); } }, - }); + })); registerSession({ pid: process.pid, @@ -876,7 +890,7 @@ if (args[0] === "sessions") { await Bun.sleep(1500); // Cleanup - server.stop(); + await server.stop(); // Output feedback (captured by slash command) if (result.exit) { @@ -1050,7 +1064,7 @@ if (args[0] === "sessions") { const annotateProject = (await detectProjectName()) ?? "_unknown"; // Start the annotate server (reuses plan editor HTML) - const server = await startAnnotateServer({ + const server = await serverShutdown.trackServerStart(startAnnotateServer({ markdown, filePath: absolutePath, origin: detectedOrigin, @@ -1074,7 +1088,7 @@ if (args[0] === "sessions") { project: annotateProject, htmlContent: planHtmlContent, onReady: async (url, isRemote, port) => { - handleAnnotateServerReady(url, isRemote, port); + await handleAnnotateServerReady(url, isRemote, port); if (isRemote && sharingEnabled) { if (rawHtml) { @@ -1087,7 +1101,7 @@ if (args[0] === "sessions") { } } }, - }); + })); registerSession({ pid: process.pid, @@ -1257,7 +1271,7 @@ if (args[0] === "sessions") { ? recentMessages.map((m) => ({ messageId: m.messageId, text: m.text, timestamp: m.timestamp })) : undefined; - const server = await startAnnotateServer({ + const server = await serverShutdown.trackServerStart(startAnnotateServer({ markdown: annotatedMessage.text, filePath: "last-message", origin: detectedOrigin, @@ -1274,13 +1288,13 @@ if (args[0] === "sessions") { htmlContent: planHtmlContent, recentMessages: pickerMessages, onReady: async (url, isRemote, port) => { - handleAnnotateServerReady(url, isRemote, port); + await handleAnnotateServerReady(url, isRemote, port); if (isRemote && sharingEnabled) { await writeRemoteShareLink(annotatedMessage.text, shareBaseUrl, "annotate", "message only").catch(() => {}); } }, - }); + })); registerSession({ pid: process.pid, @@ -1296,7 +1310,7 @@ if (args[0] === "sessions") { await Bun.sleep(1500); - server.stop(); + await server.stop(); emitAnnotateOutcome(result); process.exit(0); @@ -1308,17 +1322,16 @@ if (args[0] === "sessions") { const archiveProject = (await detectProjectName()) ?? "_unknown"; - const server = await startPlannotatorServer({ + const server = await serverShutdown.trackServerStart(startPlannotatorServer({ plan: "", origin: detectedOrigin, mode: "archive", sharingEnabled, shareBaseUrl, htmlContent: planHtmlContent, - onReady: (url, isRemote, port) => { - handleServerReady(url, isRemote, port); - }, - }); + onReady: (url, isRemote, port) => + handleServerReady(url, isRemote, port, { kind: "archive" }), + })); registerSession({ pid: process.pid, @@ -1333,7 +1346,7 @@ if (args[0] === "sessions") { await server.waitForDone!(); await Bun.sleep(500); - server.stop(); + await server.stop(); process.exit(0); } else if (args[0] === "opencode-plan") { @@ -1366,7 +1379,7 @@ if (args[0] === "sessions") { const bridgeSharingEnabled = getBridgeSharingEnabled(input); const bridgeShareBaseUrl = getBridgeShareBaseUrl(input); const bridgePasteApiUrl = getBridgePasteApiUrl(input); - const server = await startPlannotatorServer({ + const server = await serverShutdown.trackServerStart(startPlannotatorServer({ plan: planContent, origin: "opencode", sharingEnabled: bridgeSharingEnabled, @@ -1381,7 +1394,7 @@ if (args[0] === "sessions") { await writeRemoteShareLink(planContent, bridgeShareBaseUrl, "review the plan", "plan only").catch(() => {}); } }, - }); + })); registerSession({ pid: process.pid, @@ -1513,7 +1526,7 @@ if (args[0] === "sessions") { const bridgeShareBaseUrl = getBridgeShareBaseUrl(input); const reviewProject = (await detectProjectName()) ?? "_unknown"; - const server = await startReviewServer({ + const server = await serverShutdown.trackServerStart(startReviewServer({ rawPatch, gitRef, error: diffError, @@ -1529,10 +1542,9 @@ if (args[0] === "sessions") { shareBaseUrl: bridgeShareBaseUrl, htmlContent: reviewHtmlContent, opencodeClient: makeOpenCodeBridgeClient(input.agents), - onReady: (url, isRemote, port) => { - handleReviewServerReady(url, isRemote, port); - }, - }); + onReady: (url, isRemote, port) => + handleReviewServerReady(url, isRemote, port), + })); registerSession({ pid: process.pid, @@ -1548,7 +1560,7 @@ if (args[0] === "sessions") { const result = await server.waitForDecision(); await Bun.sleep(1500); - server.stop(); + await server.stop(); console.log(JSON.stringify({ decision: result.exit @@ -1605,7 +1617,7 @@ if (args[0] === "sessions") { const annotateProject = (await detectProjectName()) ?? "_unknown"; const pickerMessages = recentMessages.length > 1 ? recentMessages : undefined; - const server = await startAnnotateServer({ + const server = await serverShutdown.trackServerStart(startAnnotateServer({ markdown: lastMessage.text, filePath: "last-message", origin: "opencode", @@ -1617,10 +1629,9 @@ if (args[0] === "sessions") { gate: input.gate === true, approvalNotesSupported: input.gate === true, htmlContent: planHtmlContent, - onReady: (url, isRemote, port) => { - handleAnnotateServerReady(url, isRemote, port); - }, - }); + onReady: (url, isRemote, port) => + handleAnnotateServerReady(url, isRemote, port), + })); registerSession({ pid: process.pid, @@ -1634,7 +1645,7 @@ if (args[0] === "sessions") { const result = await server.waitForDecision(); await Bun.sleep(1500); - server.stop(); + await server.stop(); emitOpenCodeAnnotateOutcome(result); process.exit(0); @@ -1673,7 +1684,7 @@ if (args[0] === "sessions") { const planProject = (await detectProjectName()) ?? "_unknown"; - const server = await startPlannotatorServer({ + const server = await serverShutdown.trackServerStart(startPlannotatorServer({ plan: planContent, origin: "copilot-cli", sharingEnabled, @@ -1681,13 +1692,13 @@ if (args[0] === "sessions") { pasteApiUrl, htmlContent: planHtmlContent, onReady: async (url, isRemote, port) => { - handleServerReady(url, isRemote, port); + await handleServerReady(url, isRemote, port); if (isRemote && sharingEnabled) { await writeRemoteShareLink(planContent, shareBaseUrl, "review the plan", "plan only").catch(() => {}); } }, - }); + })); registerSession({ pid: process.pid, @@ -1701,7 +1712,7 @@ if (args[0] === "sessions") { const result = await server.waitForDecision(); await Bun.sleep(1500); - server.stop(); + await server.stop(); // Output Copilot CLI permission decision format if (result.approved) { @@ -1758,7 +1769,7 @@ if (args[0] === "sessions") { const annotateProject = (await detectProjectName()) ?? "_unknown"; const pickerMessages = recent.length > 1 ? recent : undefined; - const server = await startAnnotateServer({ + const server = await serverShutdown.trackServerStart(startAnnotateServer({ markdown: msg.text, filePath: "last-message", origin: "copilot-cli", @@ -1774,13 +1785,13 @@ if (args[0] === "sessions") { }), htmlContent: planHtmlContent, onReady: async (url, isRemote, port) => { - handleAnnotateServerReady(url, isRemote, port); + await handleAnnotateServerReady(url, isRemote, port); if (isRemote && sharingEnabled) { await writeRemoteShareLink(msg.text, shareBaseUrl, "annotate", "message only").catch(() => {}); } }, - }); + })); registerSession({ pid: process.pid, @@ -1794,7 +1805,7 @@ if (args[0] === "sessions") { const result = await server.waitForDecision(); await Bun.sleep(1500); - server.stop(); + await server.stop(); emitAnnotateOutcome(result); process.exit(0); @@ -1870,7 +1881,7 @@ if (args[0] === "sessions") { } const planProject = (await detectProjectName()) ?? "_unknown"; - const server = await startPlannotatorServer({ + const server = await serverShutdown.trackServerStart(startPlannotatorServer({ plan: latestPlan.text, origin: "codex", sharingEnabled, @@ -1878,13 +1889,13 @@ if (args[0] === "sessions") { pasteApiUrl, htmlContent: planHtmlContent, onReady: async (url, isRemote, port) => { - handleServerReady(url, isRemote, port); + await handleServerReady(url, isRemote, port); if (isRemote && sharingEnabled) { await writeRemoteShareLink(latestPlan.text, shareBaseUrl, "review the plan", "plan only").catch(() => {}); } }, - }); + })); registerSession({ pid: process.pid, @@ -1898,7 +1909,7 @@ if (args[0] === "sessions") { const result = await server.waitForDecision(); await Bun.sleep(1500); - server.stop(); + await server.stop(); if (result.approved) { console.log("{}"); @@ -1948,7 +1959,7 @@ if (args[0] === "sessions") { const planProject = (await detectProjectName()) ?? "_unknown"; // Start the plan review server - const server = await startPlannotatorServer({ + const server = await serverShutdown.trackServerStart(startPlannotatorServer({ plan: planContent, origin: isGemini ? "gemini-cli" : detectedOrigin, permissionMode, @@ -1957,13 +1968,13 @@ if (args[0] === "sessions") { pasteApiUrl, htmlContent: planHtmlContent, onReady: async (url, isRemote, port) => { - handleServerReady(url, isRemote, port); + await handleServerReady(url, isRemote, port); if (isRemote && sharingEnabled) { await writeRemoteShareLink(planContent, shareBaseUrl, "review the plan", "plan only").catch(() => {}); } }, - }); + })); registerSession({ pid: process.pid, @@ -1982,7 +1993,7 @@ if (args[0] === "sessions") { await Bun.sleep(1500); // Cleanup - server.stop(); + await server.stop(); // Output decision in the appropriate format for the harness if (isGemini) { diff --git a/apps/hook/server/server-shutdown.test.ts b/apps/hook/server/server-shutdown.test.ts new file mode 100644 index 000000000..0df0694f8 --- /dev/null +++ b/apps/hook/server/server-shutdown.test.ts @@ -0,0 +1,120 @@ +import { describe, expect, test } from "bun:test"; +import { createServerShutdownCoordinator } from "./server-shutdown"; + +function createDeferred(): { + promise: Promise; + resolve: () => void; +} { + let resolve!: () => void; + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise; + }); + return { promise, resolve }; +} + +describe("createServerShutdownCoordinator", () => { + test("awaits the active server before exiting on the first signal", async () => { + const events: string[] = []; + const stop = createDeferred(); + const coordinator = createServerShutdownCoordinator({ + exit: (code) => { + events.push(`exit:${code}`); + }, + }); + + void coordinator.trackServerStart(Promise.resolve({ + stop: async () => { + events.push("stop:start"); + await stop.promise; + events.push("stop:done"); + }, + })); + + const shutdown = coordinator.handleSignal("SIGINT"); + await Promise.resolve(); + expect(events).toEqual(["stop:start"]); + + stop.resolve(); + await shutdown; + expect(events).toEqual(["stop:start", "stop:done", "exit:130"]); + }); + + test("reports stop failures and still exits with the signal code", async () => { + const events: string[] = []; + const coordinator = createServerShutdownCoordinator({ + exit: (code) => { + events.push(`exit:${code}`); + }, + onStopError: (error) => { + events.push(`error:${error instanceof Error ? error.message : error}`); + }, + }); + + void coordinator.trackServerStart(Promise.resolve({ + stop: async () => { + events.push("stop"); + throw new Error("dismiss failed"); + }, + })); + + await coordinator.handleSignal("SIGTERM"); + expect(events).toEqual([ + "stop", + "error:dismiss failed", + "exit:143", + ]); + }); + + test("tracks startup so signals during onReady still stop the server", async () => { + const events: string[] = []; + const start = createDeferred(); + const coordinator = createServerShutdownCoordinator({ + exit: (code) => { + events.push(`exit:${code}`); + }, + }); + + const serverStart = start.promise.then(() => ({ + stop: async () => { + events.push("stop"); + }, + })); + void coordinator.trackServerStart(serverStart); + + const shutdown = coordinator.handleSignal("SIGINT"); + await Promise.resolve(); + expect(events).toEqual([]); + + start.resolve(); + await shutdown; + expect(events).toEqual(["stop", "exit:130"]); + }); + + test("uses a second signal as an immediate force exit", async () => { + const events: string[] = []; + const stop = createDeferred(); + const coordinator = createServerShutdownCoordinator({ + exit: (code) => { + events.push(`exit:${code}`); + }, + }); + + void coordinator.trackServerStart(Promise.resolve({ + stop: async () => { + events.push("stop:start"); + await stop.promise; + events.push("stop:done"); + }, + })); + + const gracefulShutdown = coordinator.handleSignal("SIGINT"); + await Promise.resolve(); + await coordinator.handleSignal("SIGTERM"); + + expect(events).toEqual(["stop:start", "exit:143"]); + + stop.resolve(); + await gracefulShutdown; + expect(events).toEqual(["stop:start", "exit:143", "stop:done"]); + }); +}); diff --git a/apps/hook/server/server-shutdown.ts b/apps/hook/server/server-shutdown.ts new file mode 100644 index 000000000..3e231bb33 --- /dev/null +++ b/apps/hook/server/server-shutdown.ts @@ -0,0 +1,75 @@ +export type FatalSignal = "SIGINT" | "SIGTERM"; + +export interface StoppableServer { + stop: () => void | Promise; +} + +export interface ServerShutdownCoordinator { + trackServerStart: ( + serverStart: Promise, + ) => Promise; + handleSignal: (signal: FatalSignal) => Promise; +} + +export interface ServerShutdownCoordinatorOptions { + exit: (code: number) => void; + onStopError?: (error: unknown) => void; +} + +function exitCodeForSignal(signal: FatalSignal): number { + return signal === "SIGINT" ? 130 : 143; +} + +/** + * Coordinates process signals with the one server owned by the hook CLI. + * + * The first signal waits for the active server's idempotent stop routine so + * external presenters are dismissed before process exit. A second signal is + * deliberately treated as a force-exit escape hatch when cleanup is stuck. + */ +export function createServerShutdownCoordinator({ + exit, + onStopError = () => {}, +}: ServerShutdownCoordinatorOptions): ServerShutdownCoordinator { + let activeServer: Promise | undefined; + let shutdownStarted = false; + let forceExited = false; + + return { + trackServerStart( + serverStart: Promise, + ): Promise { + // Track the pending start, rather than only its result. A presenter can + // open from onReady before the start promise resolves, so a signal in + // that window must wait for the server object and then stop it. + activeServer = serverStart; + return serverStart; + }, + + async handleSignal(signal: FatalSignal): Promise { + const exitCode = exitCodeForSignal(signal); + + if (shutdownStarted) { + forceExited = true; + exit(exitCode); + return; + } + + shutdownStarted = true; + const server = activeServer; + + try { + const startedServer = await server; + await startedServer?.stop(); + } catch (error) { + onStopError(error); + } finally { + // With a real process, the force-exit call above never returns. The + // guard also keeps injected test exits from producing a second exit. + if (!forceExited) { + exit(exitCode); + } + } + }, + }; +} diff --git a/apps/opencode-plugin/commands.ts b/apps/opencode-plugin/commands.ts index 20128b85a..1137333ed 100644 --- a/apps/opencode-plugin/commands.ts +++ b/apps/opencode-plugin/commands.ts @@ -156,15 +156,15 @@ export async function handleReviewCommand( shareBaseUrl: getShareBaseUrl(), htmlContent: reviewHtmlContent, opencodeClient: client, - onReady: (url, isRemote, port) => { - handleReviewServerReady(url, isRemote, port); + onReady: async (url, isRemote, port) => { + await handleReviewServerReady(url, isRemote, port); client.app.log({ level: "info", message: `[Plannotator] Open code review: ${url}` }); }, }); const result = await server.waitForDecision(); await Bun.sleep(1500); - server.stop(); + await server.stop(); if (result.exit) { return; @@ -346,15 +346,15 @@ export async function handleAnnotateCommand( approvalNotesSupported: Boolean(sessionId), agentCwd, htmlContent, - onReady: (url, isRemote, port) => { - handleAnnotateServerReady(url, isRemote, port); + onReady: async (url, isRemote, port) => { + await handleAnnotateServerReady(url, isRemote, port); client.app.log({ level: "info", message: `[Plannotator] Open annotation UI: ${url}` }); }, }); const result = await server.waitForDecision(); await Bun.sleep(1500); - server.stop(); + await server.stop(); if (result.exit || (result.approved && !result.feedback)) { return; @@ -463,15 +463,15 @@ export async function handleAnnotateLastCommand( gate, approvalNotesSupported: true, htmlContent, - onReady: (url, isRemote, port) => { - handleAnnotateServerReady(url, isRemote, port); + onReady: async (url, isRemote, port) => { + await handleAnnotateServerReady(url, isRemote, port); client.app.log({ level: "info", message: `[Plannotator] Open annotation UI: ${url}` }); }, }); const result = await server.waitForDecision(); await Bun.sleep(1500); - server.stop(); + await server.stop(); if (result.exit || (result.approved && !result.feedback)) { return null; diff --git a/apps/pi-extension/index.ts b/apps/pi-extension/index.ts index 12bf5c2f0..cc1eac565 100644 --- a/apps/pi-extension/index.ts +++ b/apps/pi-extension/index.ts @@ -45,6 +45,8 @@ import { PLANNOTATOR_PLAN_APPROVED_CHANNEL, type PlannotatorPlanApprovedEvent, registerPlannotatorEventListeners, + resumePlannotatorBrowserSessions, + stopActivePlannotatorBrowserSessions, } from "./plannotator-events.ts"; import { resolveTodoProvider, type TodoProvider } from "./todo-providers/index.ts"; import { @@ -269,12 +271,17 @@ export default function plannotator(pi: ExtensionAPI): void { /** Latch: no provider found, or one sync failed. Cleared on return to idle. */ let todoProviderDisabled = false; - pi.on("session_start", (_event, ctx) => { + pi.on("session_start", async (_event, ctx) => { + await resumePlannotatorBrowserSessions(); currentPiSession.update(ctx); }); - pi.on("session_shutdown", () => { - currentPiSession.clear(); + pi.on("session_shutdown", async () => { + try { + await stopActivePlannotatorBrowserSessions(); + } finally { + currentPiSession.clear(); + } }); // ── Flags ──────────────────────────────────────────────────────────── @@ -743,13 +750,15 @@ export default function plannotator(pi: ExtensionAPI): void { absolutePath, markdown, mode ?? "annotate", - folderPath, - sourceInfo, - sourceConverted, - gate, - rawHtml, - !!rawHtml, - renderMarkdownFlag, + { + folderPath, + sourceInfo, + sourceConverted, + gate, + rawHtml, + renderHtml: !!rawHtml, + convertHtml: renderMarkdownFlag, + }, ); ctx.ui.notify(sessionOpenedMessage("Annotation opened", session.url), "info"); void session diff --git a/apps/pi-extension/plannotator-browser-runtime.ts b/apps/pi-extension/plannotator-browser-runtime.ts index e9138029e..35c8ece04 100644 --- a/apps/pi-extension/plannotator-browser-runtime.ts +++ b/apps/pi-extension/plannotator-browser-runtime.ts @@ -75,3 +75,28 @@ export function loadPlannotatorBrowser(): Promise { } return browserModulePromise; } + +/** + * Stop sessions from the browser graph if that graph has already been requested. + * + * This preserves lazy startup: a Pi session that never opened Plannotator does + * not import the server graph merely because it is shutting down. + */ +export async function stopLoadedPlannotatorBrowserSessions(): Promise { + const loadedBrowserModule = browserModulePromise; + if (!loadedBrowserModule) return; + const browser = await loadedBrowserModule; + await browser.stopActiveBrowserDecisionSessions(); +} + +/** + * Reopen browser-session starts if the browser graph was previously requested. + * + * A session that never used Plannotator still avoids importing the graph. + */ +export async function resumeLoadedPlannotatorBrowserSessions(): Promise { + const loadedBrowserModule = browserModulePromise; + if (!loadedBrowserModule) return; + const browser = await loadedBrowserModule; + await browser.resumeBrowserDecisionSessions(); +} diff --git a/apps/pi-extension/plannotator-browser.test.ts b/apps/pi-extension/plannotator-browser.test.ts index 5fa199c58..c82642b63 100644 --- a/apps/pi-extension/plannotator-browser.test.ts +++ b/apps/pi-extension/plannotator-browser.test.ts @@ -1,5 +1,50 @@ import { describe, expect, test } from "bun:test"; -import { shouldUseLocalPrCheckout } from "./plannotator-browser.ts"; +import { + openArchiveBrowserAction, + startCodeReviewBrowserSession, + startBrowserDecisionSession, + startLastMessageAnnotationSession, + startMarkdownAnnotationSession, + startPlanReviewBrowserSession, + stopActiveBrowserDecisionSessions, + shouldUseLocalPrCheckout, +} from "./plannotator-browser.ts"; +import { loadPlannotatorBrowser } from "./plannotator-browser-runtime.ts"; +import { + resumePlannotatorBrowserSessions, + stopActivePlannotatorBrowserSessions, +} from "./plannotator-events.ts"; +import { + chmodSync, + existsSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; + +const presenterFixture = fileURLToPath( + new URL( + "../../packages/shared/test-fixtures/presenter-fixture.mjs", + import.meta.url, + ), +); +if (process.platform !== "win32") chmodSync(presenterFixture, 0o755); + +async function waitForRequestCount(logPath: string, expectedCount: number): Promise { + const deadline = Date.now() + 2_000; + while (Date.now() < deadline) { + if (existsSync(logPath)) { + const requests = readFileSync(logPath, "utf8").trim().split(/\r?\n/).filter(Boolean); + if (requests.length >= expectedCount) return; + } + await new Promise((resolve) => setTimeout(resolve, 10)); + } + throw new Error(`Timed out waiting for ${expectedCount} presenter requests.`); +} describe("shouldUseLocalPrCheckout", () => { test("uses local PR checkout by default", () => { @@ -11,3 +56,387 @@ describe("shouldUseLocalPrCheckout", () => { expect(shouldUseLocalPrCheckout({ useLocal: false })).toBe(false); }); }); + +describe.skipIf(process.platform === "win32")("Pi presenter lifecycle", () => { + test("preserves the decision result and dismisses the exact presentation", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "plannotator-pi-presenter-")); + const logPath = join(tempDir, "requests.jsonl"); + const originalPresenter = process.env.PLANNOTATOR_PRESENTER; + const originalLog = process.env.PLANNOTATOR_TEST_PRESENTER_LOG; + const originalMode = process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + process.env.PLANNOTATOR_PRESENTER = presenterFixture; + process.env.PLANNOTATOR_TEST_PRESENTER_LOG = logPath; + delete process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + + let serverStops = 0; + const server = { + url: "http://localhost:45678", + stop: () => { + serverStops += 1; + }, + }; + const notifications: string[] = []; + const ctx = { + ui: { + notify: (message: string) => notifications.push(message), + }, + } as unknown as Parameters[1]; + + try { + const session = startBrowserDecisionSession( + server, + ctx, + async () => ({ approved: true, feedback: "ship it" }), + "plan", + ); + await expect(session.waitForDecision()).resolves.toEqual({ + approved: true, + feedback: "ship it", + }); + + const requests = readFileSync(logPath, "utf8") + .trim() + .split(/\r?\n/) + .map((line) => JSON.parse(line)); + expect(requests).toEqual([ + { + protocol: 1, + action: "present", + url: server.url, + kind: "plan", + }, + { + protocol: 1, + action: "dismiss", + handle: { fixture: server.url, kind: "plan" }, + }, + ]); + expect(serverStops).toBe(1); + expect(notifications).toEqual([]); + } finally { + if (originalPresenter === undefined) { + delete process.env.PLANNOTATOR_PRESENTER; + } else { + process.env.PLANNOTATOR_PRESENTER = originalPresenter; + } + if (originalLog === undefined) { + delete process.env.PLANNOTATOR_TEST_PRESENTER_LOG; + } else { + process.env.PLANNOTATOR_TEST_PRESENTER_LOG = originalLog; + } + if (originalMode === undefined) { + delete process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + } else { + process.env.PLANNOTATOR_TEST_PRESENTER_MODE = originalMode; + } + rmSync(tempDir, { recursive: true, force: true }); + } + }, 10_000); + + test("stopping a pending presentation prevents a late browser fallback", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "plannotator-pi-cancel-")); + const browserPath = join(tempDir, "browser.sh"); + const browserLog = join(tempDir, "browser.log"); + writeFileSync( + browserPath, + "#!/bin/sh\nprintf '%s\\n' \"$1\" > \"$PLANNOTATOR_TEST_BROWSER_LOG\"\n", + "utf8", + ); + chmodSync(browserPath, 0o755); + + const originalPresenter = process.env.PLANNOTATOR_PRESENTER; + const originalBrowser = process.env.BROWSER; + const originalPlannotatorBrowser = process.env.PLANNOTATOR_BROWSER; + const originalBrowserLog = process.env.PLANNOTATOR_TEST_BROWSER_LOG; + const originalMode = process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + process.env.PLANNOTATOR_PRESENTER = presenterFixture; + process.env.PLANNOTATOR_TEST_PRESENTER_MODE = "hang"; + process.env.BROWSER = browserPath; + delete process.env.PLANNOTATOR_BROWSER; + process.env.PLANNOTATOR_TEST_BROWSER_LOG = browserLog; + + let serverStops = 0; + const notifications: string[] = []; + const server = { + url: "http://localhost:45679", + stop: () => { + serverStops += 1; + }, + }; + const ctx = { + ui: { + notify: (message: string) => notifications.push(message), + }, + } as unknown as Parameters[1]; + + try { + const session = startBrowserDecisionSession( + server, + ctx, + () => new Promise(() => {}), + "plan", + ); + const firstStop = session.stop(); + const secondStop = session.stop(); + expect(secondStop).toBe(firstStop); + await firstStop; + + expect(serverStops).toBe(1); + expect(existsSync(browserLog)).toBe(false); + expect(notifications).toEqual([]); + } finally { + if (originalPresenter === undefined) { + delete process.env.PLANNOTATOR_PRESENTER; + } else { + process.env.PLANNOTATOR_PRESENTER = originalPresenter; + } + if (originalBrowser === undefined) { + delete process.env.BROWSER; + } else { + process.env.BROWSER = originalBrowser; + } + if (originalPlannotatorBrowser === undefined) { + delete process.env.PLANNOTATOR_BROWSER; + } else { + process.env.PLANNOTATOR_BROWSER = originalPlannotatorBrowser; + } + if (originalBrowserLog === undefined) { + delete process.env.PLANNOTATOR_TEST_BROWSER_LOG; + } else { + process.env.PLANNOTATOR_TEST_BROWSER_LOG = originalBrowserLog; + } + if (originalMode === undefined) { + delete process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + } else { + process.env.PLANNOTATOR_TEST_PRESENTER_MODE = originalMode; + } + rmSync(tempDir, { recursive: true, force: true }); + } + }, 10_000); + + test("the lazy shutdown wrapper awaits dismissal of every active session", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "plannotator-pi-shutdown-")); + const logPath = join(tempDir, "requests.jsonl"); + const originalPresenter = process.env.PLANNOTATOR_PRESENTER; + const originalLog = process.env.PLANNOTATOR_TEST_PRESENTER_LOG; + const originalMode = process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + process.env.PLANNOTATOR_PRESENTER = presenterFixture; + process.env.PLANNOTATOR_TEST_PRESENTER_LOG = logPath; + delete process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + + const stopCounts = [0, 0]; + const ctx = { + ui: { + notify: () => undefined, + }, + } as unknown as Parameters[1]; + + try { + // Register the dynamic browser module so the production lazy wrapper + // can clean it without changing startup behavior. + await loadPlannotatorBrowser(); + startBrowserDecisionSession( + { + url: "http://localhost:45680", + stop: () => { + stopCounts[0] += 1; + }, + }, + ctx, + () => new Promise(() => {}), + "review", + ); + startBrowserDecisionSession( + { + url: "http://localhost:45681", + stop: () => { + stopCounts[1] += 1; + }, + }, + ctx, + () => new Promise(() => {}), + "annotate", + ); + await waitForRequestCount(logPath, 2); + await new Promise((resolve) => setTimeout(resolve, 50)); + + await stopActivePlannotatorBrowserSessions(); + await stopActiveBrowserDecisionSessions(); + + const requests = readFileSync(logPath, "utf8") + .trim() + .split(/\r?\n/) + .map((line) => JSON.parse(line)); + expect(requests).toHaveLength(4); + expect(requests).toContainEqual( + { + protocol: 1, + action: "present", + url: "http://localhost:45680", + kind: "review", + }, + ); + expect(requests).toContainEqual( + { + protocol: 1, + action: "present", + url: "http://localhost:45681", + kind: "annotate", + }, + ); + expect(requests).toContainEqual( + { + protocol: 1, + action: "dismiss", + handle: { fixture: "http://localhost:45680", kind: "review" }, + }, + ); + expect(requests).toContainEqual( + { + protocol: 1, + action: "dismiss", + handle: { fixture: "http://localhost:45681", kind: "annotate" }, + }, + ); + expect(stopCounts).toEqual([1, 1]); + } finally { + await stopActiveBrowserDecisionSessions().catch(() => undefined); + await resumePlannotatorBrowserSessions(); + if (originalPresenter === undefined) { + delete process.env.PLANNOTATOR_PRESENTER; + } else { + process.env.PLANNOTATOR_PRESENTER = originalPresenter; + } + if (originalLog === undefined) { + delete process.env.PLANNOTATOR_TEST_PRESENTER_LOG; + } else { + process.env.PLANNOTATOR_TEST_PRESENTER_LOG = originalLog; + } + if (originalMode === undefined) { + delete process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + } else { + process.env.PLANNOTATOR_TEST_PRESENTER_MODE = originalMode; + } + rmSync(tempDir, { recursive: true, force: true }); + } + }, 10_000); + + test("shutdown waits for a pending server start and rejects its late session", async () => { + const tempDir = mkdtempSync(join(tmpdir(), "plannotator-pi-pending-start-")); + const logPath = join(tempDir, "requests.jsonl"); + const originalPresenter = process.env.PLANNOTATOR_PRESENTER; + const originalLog = process.env.PLANNOTATOR_TEST_PRESENTER_LOG; + const originalDataDir = process.env.PLANNOTATOR_DATA_DIR; + process.env.PLANNOTATOR_PRESENTER = presenterFixture; + process.env.PLANNOTATOR_TEST_PRESENTER_LOG = logPath; + process.env.PLANNOTATOR_DATA_DIR = join(tempDir, "data"); + + const ctx = { + cwd: tempDir, + hasUI: true, + ui: { + notify: () => undefined, + }, + } as unknown as Parameters[0]; + + try { + await resumePlannotatorBrowserSessions(); + const startup = startPlanReviewBrowserSession(ctx, "# Pending shutdown proof"); + let startupSettled = false; + const observedStartup = startup + .then( + (session) => ({ status: "resolved" as const, session }), + (error: unknown) => ({ status: "rejected" as const, error }), + ) + .then((outcome) => { + startupSettled = true; + return outcome; + }); + + await stopActivePlannotatorBrowserSessions(); + expect(startupSettled).toBe(true); + const outcome = await observedStartup; + + expect(outcome.status).toBe("rejected"); + if (outcome.status === "rejected") { + if (!(outcome.error instanceof Error)) { + throw new Error("Expected pending startup to reject with an Error."); + } + expect(outcome.error.message).toContain("shutting down"); + } + expect(existsSync(logPath)).toBe(false); + } finally { + await stopActiveBrowserDecisionSessions().catch(() => undefined); + await resumePlannotatorBrowserSessions(); + if (originalPresenter === undefined) { + delete process.env.PLANNOTATOR_PRESENTER; + } else { + process.env.PLANNOTATOR_PRESENTER = originalPresenter; + } + if (originalLog === undefined) { + delete process.env.PLANNOTATOR_TEST_PRESENTER_LOG; + } else { + process.env.PLANNOTATOR_TEST_PRESENTER_LOG = originalLog; + } + if (originalDataDir === undefined) { + delete process.env.PLANNOTATOR_DATA_DIR; + } else { + process.env.PLANNOTATOR_DATA_DIR = originalDataDir; + } + rmSync(tempDir, { recursive: true, force: true }); + } + }, 10_000); + + test("the shutdown latch rejects every entrypoint and closes direct late registrations", async () => { + const ctx = { + cwd: process.cwd(), + hasUI: true, + ui: { + notify: () => undefined, + setStatus: () => undefined, + theme: { + fg: (_color: string, message: string) => message, + }, + }, + } as unknown as Parameters[0]; + + await stopActiveBrowserDecisionSessions(); + try { + await expect(startPlanReviewBrowserSession(ctx, "# Late plan")).rejects.toThrow("shutting down"); + await expect(startCodeReviewBrowserSession(ctx)).rejects.toThrow("shutting down"); + await expect( + startMarkdownAnnotationSession(ctx, "note.md", "Late note", "annotate"), + ).rejects.toThrow("shutting down"); + await expect(startLastMessageAnnotationSession(ctx, "Late message")).rejects.toThrow( + "shutting down", + ); + await expect(openArchiveBrowserAction(ctx)).rejects.toThrow("shutting down"); + + let directServerStops = 0; + expect(() => + startBrowserDecisionSession( + { + url: "http://localhost:45682", + stop: () => { + directServerStops += 1; + }, + }, + ctx, + () => new Promise(() => {}), + "plan", + ), + ).toThrow("shutting down"); + expect(directServerStops).toBe(1); + } finally { + await resumePlannotatorBrowserSessions(); + } + + const unavailableCtx = { + ...ctx, + hasUI: false, + } as unknown as Parameters[0]; + await expect(startPlanReviewBrowserSession(unavailableCtx, "# Next session")).rejects.toThrow( + "unavailable", + ); + }); +}); diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index a58b73d0f..fd4b71e91 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -34,6 +34,11 @@ import { import { parseRemoteUrl } from "./generated/repo.ts"; import { fetchRef, createWorktree, removeWorktree, ensureObjectAvailable } from "./generated/worktree.ts"; import { loadConfig, resolveDefaultDiffType, resolveSharingEnabled } from "./generated/config.ts"; +import { + presentUrl, + type ExternalPresentation, + type PresentationKind, +} from "./generated/presenter.ts"; import { WorkspaceReviewSession, type WorkspaceDiffType, @@ -45,6 +50,7 @@ import { hasPlanBrowserHtml, hasReviewBrowserHtml, } from "./plannotator-browser-runtime.ts"; +import type { PiAnnotateDecision } from "./annotate-outcome.ts"; export { getLastAssistantMessageText } from "./assistant-message.ts"; export { getStartupErrorMessage, @@ -64,7 +70,8 @@ export interface PlanReviewDecision { export interface BrowserDecisionSession { url: string; waitForDecision: () => Promise; - stop: () => void; + /** Stop the server and resolve only after any external presentation is dismissed. */ + stop: () => Promise; } type CodeReviewOptions = { @@ -84,7 +91,34 @@ type CodeReviewDecision = { exit?: boolean; }; +type RecentMessage = { + messageId: string; + text: string; + timestamp?: string; +}; + +type MarkdownAnnotationOptions = { + folderPath?: string; + sourceInfo?: string; + sourceConverted?: boolean; + gate?: boolean; + rawHtml?: string; + renderHtml?: boolean; + convertHtml?: boolean; + recentMessages?: RecentMessage[]; +}; + const CODE_REVIEW_PROGRESS_STATUS = "plannotator-review"; +const BROWSER_SESSIONS_SHUTTING_DOWN_MESSAGE = "Plannotator browser sessions are shutting down."; + +type ActiveBrowserDecisionSession = { + stop: () => Promise; +}; + +const activeBrowserDecisionSessions = new Set(); +const pendingBrowserDecisionSessionStarts = new Set>(); +let activeBrowserDecisionSessionsCleanup: Promise | undefined; +let browserDecisionSessionStartsAllowed = true; function setCodeReviewProgress(ctx: ExtensionContext, message?: string): void { ctx.ui.setStatus( @@ -102,13 +136,38 @@ function delay(ms: number): Promise { return new Promise((resolvePromise) => setTimeout(resolvePromise, ms)); } -async function openBrowserForServer(serverUrl: string, ctx: ExtensionContext): Promise { +async function openBrowserForServer( + serverUrl: string, + ctx: ExtensionContext, + kind: PresentationKind, + signal?: AbortSignal, +): Promise { + if (signal?.aborted) return undefined; + const presented = await presentUrl(serverUrl, kind, { signal }); + if (presented.opened) { + if (signal?.aborted) { + await presented.presentation.dismiss(); + return undefined; + } + return presented.presentation; + } + if (signal?.aborted) return undefined; + if (presented.attempted) { + ctx.ui.notify( + `External presenter failed; opening the default browser instead: ${presented.error}`, + "warning", + ); + } + if (signal?.aborted) return undefined; + const browserResult = await openBrowser(serverUrl); + if (signal?.aborted) return undefined; if (isRemoteSession()) { ctx.ui.notify(`[Plannotator] ${serverUrl}`, "info"); } else if (!browserResult.opened) { ctx.ui.notify(`Open this URL to review: ${serverUrl}`, "info"); } + return undefined; } async function buildLocalWorkspaceReview( @@ -129,47 +188,150 @@ async function buildLocalWorkspaceReview( }, root, options); } -async function openBrowserAndWait( - server: { url: string; stop: () => void }, - ctx: ExtensionContext, - waitForResult: () => Promise, -): Promise { - await openBrowserForServer(server.url, ctx); - return waitForDecisionWithCleanup(server, waitForResult); +function createBrowserSessionsShuttingDownError(): Error { + return new Error(BROWSER_SESSIONS_SHUTTING_DOWN_MESSAGE); } -async function waitForDecisionWithCleanup( - server: { url: string; stop: () => void }, - waitForResult: () => Promise, -): Promise { +function trackBrowserDecisionSessionStart(start: () => Promise): Promise { + if (!browserDecisionSessionStartsAllowed) { + return Promise.reject(createBrowserSessionsShuttingDownError()); + } + + let startPromise: Promise; try { - const result = await waitForResult(); - await delay(1500); - return result; - } finally { - server.stop(); + startPromise = start(); + } catch (error) { + return Promise.reject(error); } + pendingBrowserDecisionSessionStarts.add(startPromise); + void startPromise.then( + () => { + pendingBrowserDecisionSessionStarts.delete(startPromise); + }, + () => { + pendingBrowserDecisionSessionStarts.delete(startPromise); + }, + ); + return startPromise; } -function startBrowserDecisionSession( +/** + * Stop every browser decision session owned by the Pi extension. + * + * The shutdown latch is set before pending starts are awaited, so late starts + * are rejected and already-pending starts cannot register an orphaned session. + * Concurrent cleanup calls share one promise. + */ +export function stopActiveBrowserDecisionSessions(): Promise { + browserDecisionSessionStartsAllowed = false; + if (activeBrowserDecisionSessionsCleanup) { + return activeBrowserDecisionSessionsCleanup; + } + + const cleanup = (async () => { + const errors: unknown[] = []; + await Promise.allSettled([...pendingBrowserDecisionSessionStarts]); + while (activeBrowserDecisionSessions.size > 0) { + const sessions = [...activeBrowserDecisionSessions]; + const results = await Promise.allSettled(sessions.map((session) => session.stop())); + for (const result of results) { + if (result.status === "rejected") errors.push(result.reason); + } + } + + if (errors.length === 1) throw errors[0]; + if (errors.length > 1) { + throw new AggregateError(errors, "Failed to stop every active Plannotator browser session."); + } + })(); + activeBrowserDecisionSessionsCleanup = cleanup.finally(() => { + activeBrowserDecisionSessionsCleanup = undefined; + }); + return activeBrowserDecisionSessionsCleanup; +} + +/** + * Allow browser session starts for the next Pi session. + * + * If called while prior-session cleanup is still settling, cleanup retains + * ownership and is allowed to finish before the latch is reopened. + */ +export async function resumeBrowserDecisionSessions(): Promise { + const cleanup = activeBrowserDecisionSessionsCleanup; + if (cleanup) { + try { + await cleanup; + } catch { + // The session_shutdown caller owns reporting the prior cleanup failure. + } + } + browserDecisionSessionStartsAllowed = true; +} + +/** Create and register one browser decision session with awaitable, idempotent cleanup. */ +export function startBrowserDecisionSession( server: { url: string; stop: () => void }, ctx: ExtensionContext, waitForResult: () => Promise, + kind: PresentationKind, ): BrowserDecisionSession { - openBrowserForServer(server.url, ctx); + if (!browserDecisionSessionStartsAllowed) { + const shutdownError = createBrowserSessionsShuttingDownError(); + try { + server.stop(); + } catch (stopError) { + throw new AggregateError( + [shutdownError, stopError], + "Failed to reject and close a late Plannotator browser session.", + ); + } + throw shutdownError; + } + + const presentationAbort = new AbortController(); + const presentationPromise = openBrowserForServer( + server.url, + ctx, + kind, + presentationAbort.signal, + ).catch(() => undefined); let stopped = false; let stopReject: ((err: Error) => void) | undefined; let decisionPromise: Promise | undefined; + let stopPromise: Promise | undefined; const createStoppedError = () => new Error("Plannotator browser session was stopped."); - const stop = () => { - if (stopped) return; + let session: BrowserDecisionSession; + const stop = (): Promise => { + if (stopPromise) return stopPromise; stopped = true; - server.stop(); + presentationAbort.abort(); + const cleanupErrors: unknown[] = []; + try { + server.stop(); + } catch (error) { + cleanupErrors.push(error); + } stopReject?.(createStoppedError()); stopReject = undefined; + stopPromise = (async () => { + try { + const presentation = await presentationPromise; + await presentation?.dismiss(); + } catch (error) { + cleanupErrors.push(error); + } finally { + activeBrowserDecisionSessions.delete(session); + } + + if (cleanupErrors.length === 1) throw cleanupErrors[0]; + if (cleanupErrors.length > 1) { + throw new AggregateError(cleanupErrors, "Failed to stop the Plannotator browser session."); + } + })(); + return stopPromise; }; - return { + session = { url: server.url, waitForDecision: () => { if (decisionPromise) return decisionPromise; @@ -184,46 +346,52 @@ function startBrowserDecisionSession( await delay(1500); return result; } finally { - stop(); + await stop(); } })(); return decisionPromise; }, stop, }; + activeBrowserDecisionSessions.add(session); + return session; } -export async function startPlanReviewBrowserSession( +/** Start a tracked plan-review session unless Pi is shutting the current session down. */ +export function startPlanReviewBrowserSession( ctx: ExtensionContext, planContent: string, ): Promise { - if (!ctx.hasUI) { - throw new Error("Plannotator browser review is unavailable in this session."); - } - const planHtmlContent = getPlanBrowserHtml(); - if (!planHtmlContent) { - throw new Error("Plannotator browser review is unavailable in this session."); - } - - const server = await startPlanReviewServer({ - plan: planContent, - htmlContent: planHtmlContent, - origin: "pi", - sharingEnabled: resolveSharingEnabled(loadConfig()), - shareBaseUrl: process.env.PLANNOTATOR_SHARE_URL || undefined, - pasteApiUrl: process.env.PLANNOTATOR_PASTE_URL || undefined, - }); + return trackBrowserDecisionSessionStart(async () => { + if (!ctx.hasUI) { + throw new Error("Plannotator browser review is unavailable in this session."); + } + const planHtmlContent = getPlanBrowserHtml(); + if (!planHtmlContent) { + throw new Error("Plannotator browser review is unavailable in this session."); + } - const session = startBrowserDecisionSession(server, ctx, server.waitForDecision); - server.onDecision(() => { - setTimeout(() => session.stop(), 1500); + const server = await startPlanReviewServer({ + plan: planContent, + htmlContent: planHtmlContent, + origin: "pi", + sharingEnabled: resolveSharingEnabled(loadConfig()), + shareBaseUrl: process.env.PLANNOTATOR_SHARE_URL || undefined, + pasteApiUrl: process.env.PLANNOTATOR_PASTE_URL || undefined, + }); + + const session = startBrowserDecisionSession(server, ctx, server.waitForDecision, "plan"); + server.onDecision(async () => { + await delay(1500); + await session.stop(); + }); + + return { + ...session, + reviewId: server.reviewId, + onDecision: server.onDecision, + }; }); - - return { - ...session, - reviewId: server.reviewId, - onDecision: server.onDecision, - }; } export async function openPlanReviewBrowser( @@ -246,15 +414,18 @@ export async function openCodeReview( return session.waitForDecision(); } -export async function startCodeReviewBrowserSession( +/** Start a tracked code-review session unless Pi is shutting the current session down. */ +export function startCodeReviewBrowserSession( ctx: ExtensionContext, options: CodeReviewOptions = {}, ): Promise> { - try { - return await createCodeReviewBrowserSession(ctx, options); - } finally { - setCodeReviewProgress(ctx); - } + return trackBrowserDecisionSessionStart(async () => { + try { + return await createCodeReviewBrowserSession(ctx, options); + } finally { + setCodeReviewProgress(ctx); + } + }); } async function createCodeReviewBrowserSession( @@ -519,7 +690,7 @@ async function createCodeReviewBrowserSession( onCleanup: worktreeCleanup, }); - return startBrowserDecisionSession(server, ctx, server.waitForDecision); + return startBrowserDecisionSession(server, ctx, server.waitForDecision, "review"); } export async function openMarkdownAnnotation( @@ -531,34 +702,52 @@ export async function openMarkdownAnnotation( sourceInfo?: string, sourceConverted?: boolean, gate?: boolean, -): Promise<{ feedback: string; exit?: boolean; approved?: boolean; selectedMessageId?: string; feedbackScope?: "message" | "messages" }> { +): Promise { const session = await startMarkdownAnnotationSession( ctx, filePath, markdown, mode, - folderPath, - sourceInfo, - sourceConverted, - gate, + { folderPath, sourceInfo, sourceConverted, gate }, ); return session.waitForDecision(); } -export async function startMarkdownAnnotationSession( +/** Start a tracked markdown-annotation session unless Pi is shutting down. */ +export function startMarkdownAnnotationSession( ctx: ExtensionContext, filePath: string, markdown: string, mode: AnnotateMode, - folderPath?: string, - sourceInfo?: string, - sourceConverted?: boolean, - gate?: boolean, - rawHtml?: string, - renderHtml?: boolean, - convertHtml?: boolean, - recentMessages?: { messageId: string; text: string; timestamp?: string }[], -): Promise> { + options: MarkdownAnnotationOptions = {}, +): Promise> { + return trackBrowserDecisionSessionStart(() => + createMarkdownAnnotationSession( + ctx, + filePath, + markdown, + mode, + options, + ), + ); +} + +async function createMarkdownAnnotationSession( + ctx: ExtensionContext, + filePath: string, + markdown: string, + mode: AnnotateMode, + { + folderPath, + sourceInfo, + sourceConverted, + gate, + rawHtml, + renderHtml, + convertHtml, + recentMessages, + }: MarkdownAnnotationOptions, +): Promise> { if (!ctx.hasUI) { throw new Error("Plannotator annotation browser is unavailable in this session."); } @@ -601,38 +790,34 @@ export async function startMarkdownAnnotationSession( project: detectProjectName(), }); - return startBrowserDecisionSession(server, ctx, server.waitForDecision); + return startBrowserDecisionSession(server, ctx, server.waitForDecision, "annotate"); } export async function openLastMessageAnnotation( ctx: ExtensionContext, lastText: string, gate?: boolean, - recentMessages?: { messageId: string; text: string; timestamp?: string }[], -): Promise<{ feedback: string; exit?: boolean; approved?: boolean; selectedMessageId?: string; feedbackScope?: "message" | "messages" }> { + recentMessages?: RecentMessage[], +): Promise { const session = await startLastMessageAnnotationSession(ctx, lastText, gate, recentMessages); return session.waitForDecision(); } -export async function startLastMessageAnnotationSession( +/** Start a tracked last-message session unless Pi is shutting the current session down. */ +export function startLastMessageAnnotationSession( ctx: ExtensionContext, lastText: string, gate?: boolean, - recentMessages?: { messageId: string; text: string; timestamp?: string }[], -): Promise> { - return startMarkdownAnnotationSession( - ctx, - "last-message", - lastText, - "annotate-last", - undefined, - undefined, - undefined, - gate, - undefined, - undefined, - undefined, - recentMessages, + recentMessages?: RecentMessage[], +): Promise> { + return trackBrowserDecisionSessionStart(() => + createMarkdownAnnotationSession( + ctx, + "last-message", + lastText, + "annotate-last", + { gate, recentMessages }, + ), ); } @@ -640,29 +825,32 @@ export async function openArchiveBrowserAction( ctx: ExtensionContext, customPlanPath?: string, ): Promise<{ opened: boolean }> { - if (!ctx.hasUI) { - throw new Error("Plannotator archive browser is unavailable in this session."); - } - const planHtmlContent = getPlanBrowserHtml(); - if (!planHtmlContent) { - throw new Error("Plannotator archive browser is unavailable in this session."); - } - - const server = await startPlanReviewServer({ - plan: "", - htmlContent: planHtmlContent, - origin: "pi", - mode: "archive", - customPlanPath, - sharingEnabled: resolveSharingEnabled(loadConfig()), - shareBaseUrl: process.env.PLANNOTATOR_SHARE_URL || undefined, - pasteApiUrl: process.env.PLANNOTATOR_PASTE_URL || undefined, - }); - - return openBrowserAndWait(server, ctx, async () => { - if (server.waitForDone) { - await server.waitForDone(); + const session = await trackBrowserDecisionSessionStart(async () => { + if (!ctx.hasUI) { + throw new Error("Plannotator archive browser is unavailable in this session."); + } + const planHtmlContent = getPlanBrowserHtml(); + if (!planHtmlContent) { + throw new Error("Plannotator archive browser is unavailable in this session."); } - return { opened: true }; + + const server = await startPlanReviewServer({ + plan: "", + htmlContent: planHtmlContent, + origin: "pi", + mode: "archive", + customPlanPath, + sharingEnabled: resolveSharingEnabled(loadConfig()), + shareBaseUrl: process.env.PLANNOTATOR_SHARE_URL || undefined, + pasteApiUrl: process.env.PLANNOTATOR_PASTE_URL || undefined, + }); + + return startBrowserDecisionSession(server, ctx, async () => { + if (server.waitForDone) { + await server.waitForDone(); + } + return { opened: true }; + }, "archive"); }); + return session.waitForDecision(); } diff --git a/apps/pi-extension/plannotator-events.ts b/apps/pi-extension/plannotator-events.ts index 3e3012b9a..26645370e 100644 --- a/apps/pi-extension/plannotator-events.ts +++ b/apps/pi-extension/plannotator-events.ts @@ -12,6 +12,8 @@ import { hasPlanBrowserHtml, hasReviewBrowserHtml, loadPlannotatorBrowser, + resumeLoadedPlannotatorBrowserSessions, + stopLoadedPlannotatorBrowserSessions, } from "./plannotator-browser-runtime.ts"; type PlannotatorBrowserModule = typeof import("./plannotator-browser.ts"); @@ -79,6 +81,16 @@ export function openArchiveBrowserAction( return loadPlannotatorBrowser().then((browser) => browser.openArchiveBrowserAction(...args)); } +/** Stop every active Pi Plannotator browser session without defeating lazy startup. */ +export function stopActivePlannotatorBrowserSessions(): Promise { + return stopLoadedPlannotatorBrowserSessions(); +} + +/** Reopen Pi Plannotator browser starts without defeating lazy startup. */ +export function resumePlannotatorBrowserSessions(): Promise { + return resumeLoadedPlannotatorBrowserSessions(); +} + export const PLANNOTATOR_REQUEST_CHANNEL = "plannotator:request" as const; export const PLANNOTATOR_REVIEW_RESULT_CHANNEL = "plannotator:review-result" as const; export const PLANNOTATOR_PLAN_APPROVED_CHANNEL = "plannotator:plan-approved" as const; diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index 845197618..6a5201a33 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -29,7 +29,7 @@ for f in config-types storage-types workspace-status-types; do done # Everything else in the original flat list stays sourced from packages/shared. -for f in prompts review-core diff-paths cli-pagination jj-core gitbutler-core vcs-core review-args draft annotate-history pr-types pr-context-live pr-artifact-document pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common resolve-file annotate-reference-roots-node worktree worktree-pool html-to-markdown html-diff html-assets html-assets-node url-to-markdown tour annotate-args at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff single-flight source-save-node review-profiles guide guide-store commit-avatars commit-history port-range; do +for f in prompts review-core diff-paths cli-pagination jj-core gitbutler-core vcs-core review-args draft annotate-history pr-types pr-context-live pr-artifact-document pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common resolve-file annotate-reference-roots-node worktree worktree-pool html-to-markdown html-diff html-assets html-assets-node url-to-markdown tour annotate-args at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff single-flight source-save-node review-profiles guide guide-store commit-avatars commit-history port-range presenter; do src="../../packages/shared/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/shared/%s.ts\n' "$f" | cat - "$src" > "generated/$f.ts" done diff --git a/apps/review/server/index.ts b/apps/review/server/index.ts index 8ed15b652..f479c8fbb 100644 --- a/apps/review/server/index.ts +++ b/apps/review/server/index.ts @@ -66,8 +66,8 @@ const server = await startReviewServer({ rawPatch, gitRef: displayRef, htmlContent, - onReady: (url, isRemote, port) => { - handleReviewServerReady(url, isRemote, port); + onReady: async (url, isRemote, port) => { + await handleReviewServerReady(url, isRemote, port); console.error(`Code review at ${url}`); if (isRemote) { console.error(`(Remote mode detected — if no browser opens automatically, use the URL above)`); @@ -82,7 +82,7 @@ const result = await server.waitForDecision(); await Bun.sleep(500); // Cleanup -server.stop(); +await server.stop(); // Output the feedback as JSON console.log( diff --git a/packages/server/annotate.ts b/packages/server/annotate.ts index 56095d81c..234d9e295 100644 --- a/packages/server/annotate.ts +++ b/packages/server/annotate.ts @@ -14,7 +14,7 @@ import { isRemoteSession, getServerHostname, startBunServerOnAvailablePort } from "./remote"; import { getRepoInfo } from "./repo"; import type { Origin } from "@plannotator/shared/agents"; -import { handleImage, handleUpload, handleServerReady, handleDraftSave, handleDraftLoad, handleDraftDelete, handleApiNotFound, handleFavicon, handleSaveNotes, readDraftGenerationFromBody, readDraftGenerationFromUrl } from "./shared-handlers"; +import { handleImage, handleUpload, handleServerReady, handleDraftSave, handleDraftLoad, handleDraftDelete, handleApiNotFound, handleFavicon, handleSaveNotes, readDraftGenerationFromBody, readDraftGenerationFromUrl, type ServerReadyOptions } from "./shared-handlers"; import { handleDoc, handleDocExists, handleFileBrowserFiles, handleObsidianVaults, handleObsidianFiles, handleObsidianDoc, resolveAllowedDocPath, type FolderAnnotateHistory } from "./reference-handlers"; import { handleFileBrowserFilesStream } from "./reference-watch"; import { resolveUserPath, warmFileListCache } from "@plannotator/shared/resolve-file"; @@ -45,11 +45,22 @@ import { isAIEndpointPath, type AIEndpoints } from "@plannotator/ai"; import { createHtmlAssetRegistry } from "./html-assets"; import { createBunAgentTerminalBridge } from "./agent-terminal"; import { isAgentTerminalWsRoute, supportsAnnotateAgentTerminalMode } from "@plannotator/shared/agent-terminal"; +import { takePresentation } from "@plannotator/shared/presenter"; // Re-export utilities export { isRemoteSession, getServerPort } from "./remote"; export { openBrowser } from "./browser"; -export { handleServerReady as handleAnnotateServerReady } from "./shared-handlers"; +export function handleAnnotateServerReady( + url: string, + isRemote: boolean, + port: number, + options: ServerReadyOptions = {}, +): Promise { + return handleServerReady(url, isRemote, port, { + ...options, + kind: "annotate", + }); +} // --- Types --- @@ -100,7 +111,7 @@ export interface AnnotateServerOptions { /** Project name for keying per-file version history (powers the annotate version diff). */ project?: string; /** Called when server starts with the URL, remote status, and port */ - onReady?: (url: string, isRemote: boolean, port: number) => void; + onReady?: (url: string, isRemote: boolean, port: number) => void | Promise; } export interface AnnotateServerResult { @@ -120,7 +131,7 @@ export interface AnnotateServerResult { feedbackScope?: "message" | "messages"; }>; /** Stop the server */ - stop: () => void; + stop: () => Promise; } // --- Server Implementation --- @@ -810,6 +821,27 @@ export async function startAnnotateServer( const port = server.port!; const serverUrl = `http://localhost:${port}`; + let stopPromise: Promise | undefined; + const stop = () => { + if (stopPromise) return stopPromise; + const presentation = takePresentation(serverUrl); + stopPromise = (async () => { + try { + try { + aiRuntime?.dispose(); + } finally { + agentTerminal.dispose(); + } + } finally { + try { + await server.stop(true); + } finally { + await presentation?.dismiss(); + } + } + })(); + return stopPromise; + }; // The cache warm must never gate the listening socket. Its async filesystem // walk yields between directories while requests remain serviceable. @@ -817,7 +849,12 @@ export async function startAnnotateServer( // Notify caller that server is ready if (onReady) { - onReady(serverUrl, isRemote, port); + try { + await onReady(serverUrl, isRemote, port); + } catch (error) { + await stop(); + throw error; + } } return { @@ -825,10 +862,6 @@ export async function startAnnotateServer( url: serverUrl, isRemote, waitForDecision: () => decisionPromise, - stop: () => { - aiRuntime?.dispose(); - agentTerminal.dispose(); - server.stop(); - }, + stop, }; } diff --git a/packages/server/goal-setup.ts b/packages/server/goal-setup.ts index 840d2f3b6..0bbafb9ac 100644 --- a/packages/server/goal-setup.ts +++ b/packages/server/goal-setup.ts @@ -22,17 +22,29 @@ import { handleImage, handleServerReady, handleUpload, + type ServerReadyOptions, } from "./shared-handlers"; import { detectGitUser, getServerConfig, saveConfig } from "./config"; import { isWSL } from "./browser"; - -export { handleServerReady as handleGoalSetupServerReady } from "./shared-handlers"; +import { takePresentation } from "@plannotator/shared/presenter"; + +export function handleGoalSetupServerReady( + url: string, + isRemote: boolean, + port: number, + options: ServerReadyOptions = {}, +): Promise { + return handleServerReady(url, isRemote, port, { + ...options, + kind: "goal-setup", + }); +} export interface GoalSetupServerOptions { bundle: GoalSetupBundle; htmlContent: string; origin?: Origin; - onReady?: (url: string, isRemote: boolean, port: number) => void; + onReady?: (url: string, isRemote: boolean, port: number) => void | Promise; } export interface GoalSetupServerResult { @@ -43,7 +55,7 @@ export interface GoalSetupServerResult { result?: GoalSetupResult; exit?: boolean; }>; - stop: () => void; + stop: () => Promise; } function coerceAnswers(body: unknown): GoalSetupQuestionAnswer[] { @@ -205,13 +217,34 @@ export async function startGoalSetupServer( const port = server.port!; const serverUrl = `http://localhost:${port}`; - onReady?.(serverUrl, isRemote, port); + let stopPromise: Promise | undefined; + const stop = () => { + if (stopPromise) return stopPromise; + const presentation = takePresentation(serverUrl); + stopPromise = (async () => { + try { + await server.stop(true); + } finally { + await presentation?.dismiss(); + } + })(); + return stopPromise; + }; + + if (onReady) { + try { + await onReady(serverUrl, isRemote, port); + } catch (error) { + await stop(); + throw error; + } + } return { port, url: serverUrl, isRemote, waitForDecision: () => decisionPromise, - stop: () => server.stop(), + stop, }; } diff --git a/packages/server/index.ts b/packages/server/index.ts index 07c161f8f..8c4ea4c98 100644 --- a/packages/server/index.ts +++ b/packages/server/index.ts @@ -54,6 +54,7 @@ import { createExternalAnnotationHandler } from "./external-annotations"; import { isWSL } from "./browser"; import { AI_QUERY_ENDPOINT, createAIRuntime } from "./ai-runtime"; import { isAIEndpointPath, type AIEndpoints } from "@plannotator/ai"; +import { takePresentation } from "@plannotator/shared/presenter"; // Re-export utilities export { isRemoteSession, getServerPort } from "./remote"; @@ -589,11 +590,17 @@ export async function startPlannotatorServer( const serverUrl = `http://localhost:${port}`; let stopPromise: Promise | undefined; const stop = () => { - stopPromise ??= (async () => { + if (stopPromise) return stopPromise; + const presentation = takePresentation(serverUrl); + stopPromise = (async () => { try { aiRuntime?.dispose(); } finally { - await server.stop(true); + try { + await server.stop(true); + } finally { + await presentation?.dismiss(); + } } })(); return stopPromise; diff --git a/packages/server/port-startup-compat.test.ts b/packages/server/port-startup-compat.test.ts index 12ea86b46..391374a48 100644 --- a/packages/server/port-startup-compat.test.ts +++ b/packages/server/port-startup-compat.test.ts @@ -1,7 +1,15 @@ import { afterEach, describe, expect, test } from "bun:test"; import { createTestEnvironment } from "../../tests/helpers/environment"; import { closeServer, occupyConsecutivePorts } from "../../tests/helpers/ports"; +import { normalizeGoalSetupBundle } from "@plannotator/shared/goal-setup"; +import { + dismissPresentation, + trackPresentation, +} from "@plannotator/shared/presenter"; +import { startAnnotateServer } from "./annotate"; +import { startGoalSetupServer } from "./goal-setup"; import { startPlannotatorServer } from "./index"; +import { startReviewServer } from "./review"; import { handleServerReady } from "./shared-handlers"; const envKeys = [ @@ -9,6 +17,7 @@ const envKeys = [ "PLANNOTATOR_REMOTE", "PLANNOTATOR_DATA_DIR", "PLANNOTATOR_SKIP_BROWSER_OPEN", + "PLANNOTATOR_AI", "__CFBundleIdentifier", ] as const; const environment = createTestEnvironment(envKeys, "plannotator-port-compat-"); @@ -81,6 +90,94 @@ describe("Bun startup port compatibility", () => { } }); + test("server stop dismisses the presentation it owns", async () => { + environment.reset(); + process.env.PLANNOTATOR_REMOTE = "0"; + process.env.PLANNOTATOR_DATA_DIR = environment.makeTempDir(); + let dismissed = 0; + + const server = await startPlannotatorServer({ + plan: "# Presenter lifecycle", + origin: "codex", + htmlContent: "plan", + onReady: (url, isRemote, port) => + handleServerReady(url, isRemote, port, { + presentUrl: async () => ({ + attempted: true, + opened: true, + presentation: { + handle: { paneId: "pane-lifecycle" }, + dismiss: async () => { + dismissed += 1; + return { ok: true }; + }, + }, + }), + }), + }); + + await server.stop(); + await server.stop(); + expect(dismissed).toBe(1); + }); + + test("server stop cannot dismiss a newer presentation after its URL is reused", async () => { + environment.reset(); + const { start, servers } = await occupyConsecutivePorts(1); + await closeServer(servers[0]); + process.env.PLANNOTATOR_REMOTE = "0"; + process.env.PLANNOTATOR_PORT = String(start); + process.env.PLANNOTATOR_DATA_DIR = environment.makeTempDir(); + let firstDismissed = 0; + let replacementDismissed = 0; + + const server = await startPlannotatorServer({ + plan: "# Exact presenter ownership", + origin: "codex", + htmlContent: "plan", + onReady: (url, isRemote, port) => + handleServerReady(url, isRemote, port, { + presentUrl: async () => ({ + attempted: true, + opened: true, + presentation: { + handle: { paneId: "pane-first" }, + dismiss: async () => { + firstDismissed += 1; + return { ok: true }; + }, + }, + }), + }), + }); + + const stopping = server.stop(); + const replacementServer = Bun.serve({ + hostname: "127.0.0.1", + port: start, + fetch: () => new Response("replacement"), + }); + trackPresentation(server.url, { + handle: { paneId: "pane-replacement" }, + dismiss: async () => { + replacementDismissed += 1; + return { ok: true }; + }, + }); + + try { + await stopping; + expect(firstDismissed).toBe(1); + expect(replacementDismissed).toBe(0); + + await dismissPresentation(server.url); + expect(replacementDismissed).toBe(1); + } finally { + await dismissPresentation(server.url); + await replacementServer.stop(true); + } + }); + test("an async ready failure releases the fixed port before startup rejects", async () => { environment.reset(); const { start, servers } = await occupyConsecutivePorts(1); @@ -107,4 +204,101 @@ describe("Bun startup port compatibility", () => { }); await replacement.stop(true); }); + + const readyFailureCases: { + name: string; + start: ( + onReady: (url: string, isRemote: boolean, port: number) => Promise, + onCleanup: () => void, + ) => Promise; + }[] = [ + { + name: "annotate", + start: (onReady) => + startAnnotateServer({ + markdown: "# Annotate ready failure", + filePath: "ready-failure.md", + htmlContent: "annotate", + origin: "codex", + sharingEnabled: false, + onReady, + }), + }, + { + name: "review", + start: (onReady, onCleanup) => + startReviewServer({ + rawPatch: "", + gitRef: "HEAD", + htmlContent: "review", + origin: "codex", + sharingEnabled: false, + onReady, + onCleanup, + }), + }, + { + name: "goal setup", + start: (onReady) => + startGoalSetupServer({ + bundle: normalizeGoalSetupBundle({ + stage: "interview", + title: "Ready failure", + questions: [{ id: "scope", prompt: "Scope?" }], + }), + htmlContent: "goal setup", + origin: "codex", + onReady, + }), + }, + ]; + + for (const readyFailureCase of readyFailureCases) { + test(`${readyFailureCase.name} cleans up after an async ready failure`, async () => { + environment.reset(); + const { start, servers } = await occupyConsecutivePorts(1); + await closeServer(servers[0]); + process.env.PLANNOTATOR_REMOTE = "0"; + process.env.PLANNOTATOR_PORT = String(start); + process.env.PLANNOTATOR_DATA_DIR = environment.makeTempDir(); + process.env.PLANNOTATOR_SKIP_BROWSER_OPEN = "0"; + process.env.PLANNOTATOR_AI = "disabled"; + const readyError = new Error(`${readyFailureCase.name} handoff failed`); + let dismissed = 0; + let cleanupCalls = 0; + + await expect(readyFailureCase.start( + async (url, isRemote, port) => { + await handleServerReady(url, isRemote, port, { + presentUrl: async () => ({ + attempted: true, + opened: true, + presentation: { + handle: { paneId: `pane-${readyFailureCase.name}` }, + dismiss: async () => { + dismissed += 1; + return { ok: true }; + }, + }, + }), + }); + await Promise.resolve(); + throw readyError; + }, + () => { + cleanupCalls += 1; + }, + )).rejects.toBe(readyError); + + expect(dismissed).toBe(1); + expect(cleanupCalls).toBe(readyFailureCase.name === "review" ? 1 : 0); + + const replacement = Bun.serve({ + hostname: "127.0.0.1", + port: start, + fetch: () => new Response("reused"), + }); + await replacement.stop(true); + }); + } }); diff --git a/packages/server/review.ts b/packages/server/review.ts index 9273a4e04..631b59e16 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -61,7 +61,7 @@ import { import { type AgentJobInfo, REVIEW_OUTPUT_FAILED, getAgentJobAnnotationContext, markJobReviewFailed } from "@plannotator/shared/agent-jobs"; import { createCommitAvatarResolver } from "@plannotator/shared/commit-avatars"; import { getRepoInfo } from "./repo"; -import { handleImage, handleUpload, handleAgents, handleServerReady, handleDraftSave, handleDraftLoad, handleDraftDelete, handleApiNotFound, handleFavicon, readDraftGenerationFromBody, readDraftGenerationFromUrl, type OpencodeClient } from "./shared-handlers"; +import { handleImage, handleUpload, handleAgents, handleServerReady, handleDraftSave, handleDraftLoad, handleDraftDelete, handleApiNotFound, handleFavicon, readDraftGenerationFromBody, readDraftGenerationFromUrl, type OpencodeClient, type ServerReadyOptions } from "./shared-handlers"; import { contentHash, deleteDraft } from "./draft"; import { createEditorAnnotationHandler } from "./editor-annotations"; import { createExternalAnnotationHandler } from "./external-annotations"; @@ -112,6 +112,7 @@ import { isAIEndpointPath, type AIEndpoints } from "@plannotator/ai"; import { isWSL } from "./browser"; import { handleOpenInApps, handleOpenIn } from "./open-in"; import type { LocalWorkspaceReview, WorkspaceDiffType } from "./review-workspace"; +import { takePresentation } from "@plannotator/shared/presenter"; import { handleCodeNavResolve, extractChangedFiles } from "./code-nav"; import { discoverCuratedSkills, resolveRequestedReviewProfile, listAllSkills, enableReviewSkill } from "./review-skill-loader"; import { @@ -127,7 +128,17 @@ export { isRemoteSession, getServerPort } from "./remote"; export { openBrowser } from "./browser"; export { type DiffType, type DiffOption, type GitContext, type WorktreeInfo } from "./vcs"; export { type PRMetadata } from "./pr"; -export { handleServerReady as handleReviewServerReady } from "./shared-handlers"; +export function handleReviewServerReady( + url: string, + isRemote: boolean, + port: number, + options: ServerReadyOptions = {}, +): Promise { + return handleServerReady(url, isRemote, port, { + ...options, + kind: "review", + }); +} // --- Types --- @@ -163,7 +174,7 @@ export interface ReviewServerOptions { /** Custom base URL for share links (default: https://share.plannotator.ai) */ shareBaseUrl?: string; /** Called when server starts with the URL, remote status, and port */ - onReady?: (url: string, isRemote: boolean, port: number) => void; + onReady?: (url: string, isRemote: boolean, port: number) => void | Promise; /** OpenCode client for querying available agents (OpenCode only) */ opencodeClient?: OpencodeClient; /** PR metadata when reviewing a pull request (PR mode) */ @@ -198,7 +209,7 @@ export interface ReviewServerResult { exit?: boolean; }>; /** Stop the server */ - stop: () => void; + stop: () => Promise; } // --- Server Implementation --- @@ -2895,10 +2906,46 @@ export async function startReviewServer( serverUrl = `http://localhost:${port}`; const exitHandler = () => agentJobs.killAll(); process.once("exit", exitHandler); + let stopPromise: Promise | undefined; + const stop = () => { + if (stopPromise) return stopPromise; + const presentation = takePresentation(serverUrl); + stopPromise = (async () => { + process.removeListener("exit", exitHandler); + try { + try { + agentJobs.killAll(); + } finally { + aiRuntime?.dispose(); + } + } finally { + try { + await server.stop(true); + } finally { + try { + await presentation?.dismiss(); + } finally { + // Invoke cleanup callback (e.g., remove temp worktree) + if (options.onCleanup) { + try { + await options.onCleanup(); + } catch { /* best effort */ } + } + } + } + } + })(); + return stopPromise; + }; // Notify caller that server is ready if (onReady) { - onReady(serverUrl, isRemote, port); + try { + await onReady(serverUrl, isRemote, port); + } catch (error) { + await stop(); + throw error; + } } return { @@ -2906,18 +2953,6 @@ export async function startReviewServer( url: serverUrl, isRemote, waitForDecision: () => decisionPromise, - stop: () => { - process.removeListener("exit", exitHandler); - agentJobs.killAll(); - aiRuntime?.dispose(); - server.stop(); - // Invoke cleanup callback (e.g., remove temp worktree) - if (options.onCleanup) { - try { - const result = options.onCleanup(); - if (result instanceof Promise) result.catch(() => {}); - } catch { /* best effort */ } - } - }, + stop, }; } diff --git a/packages/server/shared-handlers.test.ts b/packages/server/shared-handlers.test.ts index 2856c46e2..add17deb3 100644 --- a/packages/server/shared-handlers.test.ts +++ b/packages/server/shared-handlers.test.ts @@ -8,6 +8,7 @@ import { isCodexDesktopHost, writeServerReadyMetadata, } from "./shared-handlers"; +import { dismissPresentation } from "@plannotator/shared/presenter"; function saveNotesRequest(body: unknown): Request { return new Request("http://localhost/api/save-notes", { @@ -137,6 +138,51 @@ describe("handleServerReady", () => { expect(opened).toBe(false); }); + test("uses an external presenter before the native browser", async () => { + let opened = false; + let dismissed = 0; + const url = "http://localhost:12346"; + + await handleServerReady(url, false, 12346, { + kind: "review", + presentUrl: async (presentedUrl, kind) => ({ + attempted: true, + opened: true, + presentation: { + handle: { paneId: "pane-1" }, + dismiss: async () => { + dismissed += 1; + return { ok: true }; + }, + }, + }), + openBrowser: async () => { + opened = true; + return true; + }, + }); + + expect(opened).toBe(false); + expect(await dismissPresentation(url)).toEqual({ ok: true }); + expect(dismissed).toBe(1); + }); + + test("falls back to the native browser when the presenter fails", async () => { + let opened = ""; + await handleServerReady("http://localhost:12347", false, 12347, { + presentUrl: async () => ({ + attempted: true, + opened: false, + error: "presenter unavailable", + }), + openBrowser: async (url) => { + opened = url; + return true; + }, + }); + expect(opened).toBe("http://localhost:12347"); + }); + // Regression: a remote session must surface a reachable URL in the terminal // regardless of URL sharing — otherwise a sharing-disabled remote user is left // with no URL and the agent hangs waiting on the review. diff --git a/packages/server/shared-handlers.ts b/packages/server/shared-handlers.ts index 1859b6c82..4b08d15b1 100644 --- a/packages/server/shared-handlers.ts +++ b/packages/server/shared-handlers.ts @@ -9,6 +9,11 @@ import { appendFileSync, mkdirSync } from "node:fs"; import { dirname } from "node:path"; import { openBrowser as openBrowserImpl } from "./browser"; +import { + presentUrl as presentUrlImpl, + trackPresentation, + type PresentationKind, +} from "@plannotator/shared/presenter"; import { validateImagePath, validateUploadExtension, UPLOAD_DIR } from "./image"; import { saveDraft, loadDraft, deleteDraft, getDraftGeneration } from "./draft"; import { FAVICON_PNG_BYTES } from "@plannotator/shared/favicon"; @@ -163,10 +168,12 @@ export function handleFavicon(): Response { }); } -interface ServerReadyOptions { +export interface ServerReadyOptions { readyFile?: string; skipBrowserOpen?: boolean; openBrowser?: typeof openBrowserImpl; + presentUrl?: typeof presentUrlImpl; + kind?: PresentationKind; } export interface ServerReadyMetadata { @@ -217,6 +224,20 @@ export async function handleServerReady( const skipBrowserOpen = options.skipBrowserOpen ?? process.env.PLANNOTATOR_SKIP_BROWSER_OPEN === "1"; if (skipBrowserOpen) return; + const presented = await (options.presentUrl ?? presentUrlImpl)( + url, + options.kind ?? "plan", + ); + if (presented.opened) { + trackPresentation(url, presented.presentation); + return; + } + if (presented.attempted) { + process.stderr.write( + `[plannotator] External presenter failed; opening the default browser: ${presented.error}\n`, + ); + } + const opened = await (options.openBrowser ?? openBrowserImpl)(url, { isRemote, useGlimpse: true }); // Local fallback lifeline: if the browser couldn't be opened (headless box, diff --git a/packages/shared/config.ts b/packages/shared/config.ts index a7bb2ba91..282b042bb 100644 --- a/packages/shared/config.ts +++ b/packages/shared/config.ts @@ -57,6 +57,16 @@ export interface PromptConfig { }; } +export interface PresenterConfig { + /** Executable implementing Plannotator's JSON presenter protocol. */ + command: string; + /** + * "herdr" (the default) only enables the presenter inside a Herdr pane. + * "always" enables it in every environment. + */ + when?: "herdr" | "always"; +} + const PROMPT_SECTIONS = ["review", "plan", "annotate"] as const; export function mergePromptConfig( @@ -88,6 +98,11 @@ export interface PlannotatorConfig { displayName?: string; diffOptions?: DiffOptions; prompts?: PromptConfig; + /** + * Optional external UI presenter. The executable receives one JSON request + * on stdin and returns one JSON response on stdout. + */ + presenter?: PresenterConfig; conventionalComments?: boolean; /** null = explicitly cleared (use defaults), undefined = not set */ conventionalLabels?: CCLabelConfig[] | null; diff --git a/packages/shared/package.json b/packages/shared/package.json index a769f59e1..c441a086f 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -67,6 +67,7 @@ "./workspace-status": "./workspace-status.ts", "./open-in-apps": "./open-in-apps.ts", "./port-range": "./port-range.ts", + "./presenter": "./presenter.ts", "./plan-review-lifecycle": "./plan-review-lifecycle.ts", "./review-profiles": "./review-profiles.ts", "./commit-avatars": "./commit-avatars.ts" diff --git a/packages/shared/presenter.test.ts b/packages/shared/presenter.test.ts new file mode 100644 index 000000000..75c9d00b4 --- /dev/null +++ b/packages/shared/presenter.test.ts @@ -0,0 +1,293 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { chmodSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { + invokePresenterCommand, + presentUrl, + resolvePresenterCommand, + type PresenterRequest, +} from "./presenter"; + +const fixture = fileURLToPath( + new URL("./test-fixtures/presenter-fixture.mjs", import.meta.url), +); +if (process.platform !== "win32") chmodSync(fixture, 0o755); + +const originalMode = process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + +afterEach(() => { + if (originalMode === undefined) { + delete process.env.PLANNOTATOR_TEST_PRESENTER_MODE; + } else { + process.env.PLANNOTATOR_TEST_PRESENTER_MODE = originalMode; + } +}); + +describe("resolvePresenterCommand", () => { + test("config presenters default to Herdr-only", () => { + const config = { presenter: { command: "/presenter" } }; + expect(resolvePresenterCommand(config, {})).toBeUndefined(); + expect(resolvePresenterCommand(config, { HERDR_ENV: "1" })).toBe( + "/presenter", + ); + }); + + test("config may opt into all environments", () => { + expect( + resolvePresenterCommand( + { presenter: { command: "/presenter", when: "always" } }, + {}, + ), + ).toBe("/presenter"); + }); + + test("the env override runs anywhere and an empty override disables config", () => { + const config = { + presenter: { command: "/configured", when: "always" as const }, + }; + expect( + resolvePresenterCommand(config, { + PLANNOTATOR_PRESENTER: "/explicit", + }), + ).toBe("/explicit"); + expect( + resolvePresenterCommand(config, { PLANNOTATOR_PRESENTER: "" }), + ).toBeUndefined(); + }); +}); + +describe("presentUrl", () => { + test("sends the exact protocol and dismisses the returned handle once", async () => { + const requests: PresenterRequest[] = []; + const result = await presentUrl("http://localhost:3210", "review", { + config: {}, + env: { PLANNOTATOR_PRESENTER: "/presenter" }, + invoke: async (_command, request) => { + requests.push(request); + return request.action === "present" + ? { + ok: true, + response: { + protocol: 1, + ok: true, + handle: { paneId: "pane-1" }, + }, + } + : { ok: true, response: { protocol: 1, ok: true } }; + }, + }); + + expect(result.opened).toBe(true); + if (!result.opened) throw new Error("expected presentation"); + expect(await result.presentation.dismiss()).toEqual({ ok: true }); + expect(await result.presentation.dismiss()).toEqual({ ok: true }); + expect(requests).toEqual([ + { + protocol: 1, + action: "present", + url: "http://localhost:3210", + kind: "review", + }, + { + protocol: 1, + action: "dismiss", + handle: { paneId: "pane-1" }, + }, + ]); + }); + + test("returns presenter failures as fallback-friendly values", async () => { + const result = await presentUrl("http://localhost:3210", "plan", { + config: {}, + env: { PLANNOTATOR_PRESENTER: "/presenter" }, + invoke: async () => ({ ok: false, error: "not available" }), + }); + expect(result).toEqual({ + attempted: true, + opened: false, + error: "not available", + }); + }); + + test("reports a failed dismissal once even when cleanup is requested again", async () => { + const errors: string[] = []; + let dismissCalls = 0; + const result = await presentUrl("http://localhost:3210", "plan", { + config: {}, + env: { PLANNOTATOR_PRESENTER: "/presenter" }, + onDismissError: (error) => errors.push(error), + invoke: async (_command, request) => { + if (request.action === "present") { + return { + ok: true, + response: { + protocol: 1, + ok: true, + handle: { paneId: "pane-1" }, + }, + }; + } + dismissCalls += 1; + return { ok: false, error: "view close failed" }; + }, + }); + + expect(result.opened).toBe(true); + if (!result.opened) throw new Error("expected presentation"); + expect(await result.presentation.dismiss()).toEqual({ + ok: false, + error: "view close failed", + }); + expect(await result.presentation.dismiss()).toEqual({ + ok: false, + error: "view close failed", + }); + expect(dismissCalls).toBe(1); + expect(errors).toEqual(["view close failed"]); + }); + + test("dismiss reuses the environment snapshot that created the handle", async () => { + const env = { + PLANNOTATOR_PRESENTER: "/presenter", + HERDR_SOCKET_PATH: "/tmp/original.sock", + }; + const socketPaths: Array = []; + const result = await presentUrl("http://localhost:3210", "plan", { + config: {}, + env, + invoke: async (_command, request, commandOptions) => { + socketPaths.push(commandOptions.env?.HERDR_SOCKET_PATH); + return request.action === "present" + ? { + ok: true, + response: { + protocol: 1, + ok: true, + handle: { paneId: "pane-1" }, + }, + } + : { ok: true, response: { protocol: 1, ok: true } }; + }, + }); + + env.HERDR_SOCKET_PATH = "/tmp/replaced.sock"; + expect(result.opened).toBe(true); + if (!result.opened) throw new Error("expected presentation"); + await result.presentation.dismiss(); + expect(socketPaths).toEqual([ + "/tmp/original.sock", + "/tmp/original.sock", + ]); + }); + + test("rejects a success response without a lifecycle handle", async () => { + const result = await presentUrl("http://localhost:3210", "plan", { + config: {}, + env: { PLANNOTATOR_PRESENTER: "/presenter" }, + invoke: async () => ({ + ok: true, + response: { protocol: 1, ok: true }, + }), + }); + expect(result).toEqual({ + attempted: true, + opened: false, + error: "presenter success response is missing handle", + }); + }); +}); + +describe.skipIf(process.platform === "win32")( + "presenter subprocess boundaries", + () => { + test("round-trips one JSON record without a shell", async () => { + const result = await invokePresenterCommand(fixture, { + protocol: 1, + action: "present", + url: "http://localhost:4321", + kind: "annotate", + }); + expect(result).toEqual({ + ok: true, + response: { + protocol: 1, + ok: true, + handle: { + fixture: "http://localhost:4321", + kind: "annotate", + }, + }, + }); + }); + + test("bounds execution time", async () => { + process.env.PLANNOTATOR_TEST_PRESENTER_MODE = "hang"; + const result = await invokePresenterCommand( + fixture, + { + protocol: 1, + action: "present", + url: "http://localhost:4321", + kind: "plan", + }, + { timeoutMs: 25 }, + ); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected timeout"); + expect(result.error).toContain("timed out"); + }); + + test("cancels a pending presenter process", async () => { + process.env.PLANNOTATOR_TEST_PRESENTER_MODE = "hang"; + const cancellation = new AbortController(); + const pending = invokePresenterCommand( + fixture, + { + protocol: 1, + action: "present", + url: "http://localhost:4321", + kind: "plan", + }, + { signal: cancellation.signal }, + ); + cancellation.abort(); + + const result = await pending; + expect(result).toEqual({ + ok: false, + error: "presenter cancelled", + }); + }); + + test("caps combined stdout and stderr", async () => { + process.env.PLANNOTATOR_TEST_PRESENTER_MODE = "flood"; + const result = await invokePresenterCommand( + fixture, + { + protocol: 1, + action: "present", + url: "http://localhost:4321", + kind: "plan", + }, + { maxOutputBytes: 1024 }, + ); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected output cap"); + expect(result.error).toContain("exceeded 1024 bytes"); + }); + + test("surfaces structured presenter errors", async () => { + process.env.PLANNOTATOR_TEST_PRESENTER_MODE = "failure"; + const result = await invokePresenterCommand(fixture, { + protocol: 1, + action: "present", + url: "http://localhost:4321", + kind: "plan", + }); + expect(result).toEqual({ + ok: false, + error: "fixture_failed: fixture refused", + }); + }); + }, +); diff --git a/packages/shared/presenter.ts b/packages/shared/presenter.ts new file mode 100644 index 000000000..4c52b8950 --- /dev/null +++ b/packages/shared/presenter.ts @@ -0,0 +1,396 @@ +/** + * External presentation protocol shared by the Bun server and Pi's Node + * runtime. The configured executable receives exactly one JSON record on stdin + * and must return exactly one JSON record on stdout. + */ + +import { spawn } from "node:child_process"; +import { loadConfig, type PlannotatorConfig } from "./config"; + +export const PRESENTER_PROTOCOL_VERSION = 1; +// Leave enough time for runtime discovery, Browser startup, and failed-startup +// pane cleanup before the outer process gives up. +export const PRESENTER_TIMEOUT_MS = 60_000; +export const PRESENTER_MAX_OUTPUT_BYTES = 64 * 1024; +const PRESENTER_TERMINATION_GRACE_MS = 10_500; + +export type PresentationKind = + | "plan" + | "review" + | "annotate" + | "archive" + | "goal-setup"; + +export type JsonValue = + | null + | boolean + | number + | string + | JsonValue[] + | { [key: string]: JsonValue }; + +export type PresenterRequest = + | { + protocol: typeof PRESENTER_PROTOCOL_VERSION; + action: "present"; + url: string; + kind: PresentationKind; + } + | { + protocol: typeof PRESENTER_PROTOCOL_VERSION; + action: "dismiss"; + handle: JsonValue; + }; + +export interface PresenterOperationResult { + ok: boolean; + error?: string; +} + +export interface ExternalPresentation { + handle: JsonValue; + dismiss: () => Promise; +} + +export type PresentUrlResult = + | { attempted: false; opened: false } + | { attempted: true; opened: false; error: string } + | { + attempted: true; + opened: true; + presentation: ExternalPresentation; + }; + +export type PresenterCommandResult = + | { ok: true; response: Record } + | { ok: false; error: string }; + +interface PresenterOptions { + config?: PlannotatorConfig; + env?: NodeJS.ProcessEnv; + invoke?: typeof invokePresenterCommand; + signal?: AbortSignal; + onDismissError?: (error: string) => void; +} + +interface PresenterCommandOptions { + timeoutMs?: number; + maxOutputBytes?: number; + signal?: AbortSignal; + env?: NodeJS.ProcessEnv; +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function responseError(response: Record): string { + const error = response.error; + if (typeof error === "string" && error.trim()) return error.trim(); + if (isRecord(error)) { + const message = typeof error.message === "string" ? error.message.trim() : ""; + const code = typeof error.code === "string" ? error.code.trim() : ""; + if (code && message) return `${code}: ${message}`; + if (message) return message; + if (code) return code; + } + return "presenter reported a failure"; +} + +function commandExitError( + code: number | null, + signal: NodeJS.Signals | null, + stderr: string, +): string { + const detail = stderr.trim(); + const status = signal + ? `terminated by ${signal}` + : `exited with status ${code ?? "unknown"}`; + return detail ? `presenter ${status}: ${detail}` : `presenter ${status}`; +} + +/** + * Resolve the configured executable without shell parsing. + * + * An explicitly-set PLANNOTATOR_PRESENTER wins and may run anywhere. An empty + * explicit value disables the config-file presenter. Config-file presenters + * default to Herdr-only so installing the integration does not change normal + * terminal/browser behavior. + */ +export function resolvePresenterCommand( + config: PlannotatorConfig, + env: NodeJS.ProcessEnv = process.env, +): string | undefined { + if (Object.prototype.hasOwnProperty.call(env, "PLANNOTATOR_PRESENTER")) { + const explicit = env.PLANNOTATOR_PRESENTER?.trim(); + return explicit || undefined; + } + + const presenter = config.presenter; + if (!presenter || typeof presenter !== "object") return undefined; + const command = typeof presenter.command === "string" + ? presenter.command.trim() + : ""; + if (!command) return undefined; + + const when = presenter.when === "always" ? "always" : "herdr"; + if (when === "herdr" && env.HERDR_ENV !== "1") return undefined; + return command; +} + +/** + * Invoke one presenter operation. Failures are returned as values so callers + * can preserve the native-browser fallback. + */ +export async function invokePresenterCommand( + command: string, + request: PresenterRequest, + options: PresenterCommandOptions = {}, +): Promise { + const timeoutMs = options.timeoutMs ?? PRESENTER_TIMEOUT_MS; + const maxOutputBytes = + options.maxOutputBytes ?? PRESENTER_MAX_OUTPUT_BYTES; + + return new Promise((resolve) => { + if (options.signal?.aborted) { + resolve({ ok: false, error: "presenter cancelled" }); + return; + } + + let child: ReturnType; + try { + child = spawn(command, [], { + shell: false, + windowsHide: true, + stdio: ["pipe", "pipe", "pipe"], + env: options.env, + }); + } catch (error) { + resolve({ + ok: false, + error: `failed to start presenter: ${ + error instanceof Error ? error.message : String(error) + }`, + }); + return; + } + + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + let outputBytes = 0; + let settled = false; + let timeout: ReturnType | undefined; + + const finish = (result: PresenterCommandResult) => { + if (settled) return; + settled = true; + if (timeout) clearTimeout(timeout); + options.signal?.removeEventListener("abort", cancel); + resolve(result); + }; + + const terminate = (error: string) => { + try { + child.kill("SIGTERM"); + } catch { + // Best effort; the operation has already failed from the caller's view. + } + const escalation = setTimeout(() => { + if (child.exitCode === null && child.signalCode === null) { + try { + child.kill("SIGKILL"); + } catch { + // The process may have exited between the state check and kill. + } + } + }, PRESENTER_TERMINATION_GRACE_MS); + escalation.unref(); + finish({ ok: false, error }); + }; + + const cancel = () => terminate("presenter cancelled"); + + const collect = (target: Buffer[], chunk: Buffer | string) => { + if (settled) return; + const bytes = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); + outputBytes += bytes.byteLength; + if (outputBytes > maxOutputBytes) { + terminate(`presenter output exceeded ${maxOutputBytes} bytes`); + return; + } + target.push(bytes); + }; + + child.stdout!.on("data", (chunk) => collect(stdoutChunks, chunk)); + child.stderr!.on("data", (chunk) => collect(stderrChunks, chunk)); + child.once("error", (error) => { + finish({ ok: false, error: `failed to start presenter: ${error.message}` }); + }); + child.once("close", (code, signal) => { + if (settled) return; + const stdout = Buffer.concat(stdoutChunks).toString("utf8").trim(); + const stderr = Buffer.concat(stderrChunks).toString("utf8").trim(); + + let response: unknown; + try { + response = JSON.parse(stdout); + } catch { + if (code !== 0 || signal) { + finish({ ok: false, error: commandExitError(code, signal, stderr) }); + } else { + finish({ + ok: false, + error: stdout + ? "presenter returned invalid JSON" + : "presenter returned no response", + }); + } + return; + } + + if (!isRecord(response)) { + finish({ ok: false, error: "presenter response must be a JSON object" }); + return; + } + if (response.protocol !== PRESENTER_PROTOCOL_VERSION) { + finish({ + ok: false, + error: `unsupported presenter protocol: ${String(response.protocol)}`, + }); + return; + } + if (response.ok !== true) { + finish({ ok: false, error: responseError(response) }); + return; + } + if (code !== 0 || signal) { + finish({ ok: false, error: commandExitError(code, signal, stderr) }); + return; + } + finish({ ok: true, response }); + }); + + timeout = setTimeout(() => { + terminate(`presenter timed out after ${timeoutMs}ms`); + }, timeoutMs); + timeout.unref(); + options.signal?.addEventListener("abort", cancel, { once: true }); + if (options.signal?.aborted) { + cancel(); + } + + child.stdin!.on("error", () => { + // The close/error event carries the authoritative process result. + }); + child.stdin!.end(`${JSON.stringify(request)}\n`); + }); +} + +/** + * Present a URL with the configured external presenter. + */ +export async function presentUrl( + url: string, + kind: PresentationKind, + options: PresenterOptions = {}, +): Promise { + // Keep the presenter lifecycle on one stable environment snapshot. This + // matters for long-lived runtimes and also guarantees that dismiss uses the + // same Herdr session/context that created the handle. + const env = { ...(options.env ?? process.env) }; + const config = options.config ?? loadConfig(); + const command = resolvePresenterCommand(config, env); + if (!command) return { attempted: false, opened: false }; + + const invoke = options.invoke ?? invokePresenterCommand; + const result = await invoke(command, { + protocol: PRESENTER_PROTOCOL_VERSION, + action: "present", + url, + kind, + }, { + signal: options.signal, + env, + }); + if (!result.ok) { + return { attempted: true, opened: false, error: result.error }; + } + if (!Object.prototype.hasOwnProperty.call(result.response, "handle")) { + return { + attempted: true, + opened: false, + error: "presenter success response is missing handle", + }; + } + + const handle = result.response.handle as JsonValue; + const reportDismissError = options.onDismissError ?? ((error: string) => { + process.stderr.write( + `[plannotator] External presenter cleanup failed: ${error}\n`, + ); + }); + let dismissPromise: Promise | undefined; + const presentation: ExternalPresentation = { + handle, + dismiss: () => { + dismissPromise ??= (async () => { + const dismissed = await invoke(command, { + protocol: PRESENTER_PROTOCOL_VERSION, + action: "dismiss", + handle, + }, { + env, + }); + const result = dismissed.ok + ? { ok: true } + : { ok: false, error: dismissed.error }; + if (!result.ok) { + reportDismissError(result.error ?? "presenter cleanup failed"); + } + return result; + })(); + return dismissPromise; + }, + }; + + return { attempted: true, opened: true, presentation }; +} + +const activePresentations = new Map(); + +/** Associate a successfully opened presentation with its server URL. */ +export function trackPresentation( + url: string, + presentation: ExternalPresentation, +): void { + const previous = activePresentations.get(url); + activePresentations.set(url, presentation); + if (previous && previous !== presentation) { + void previous.dismiss(); + } +} + +/** + * Remove and return the presentation currently owned by a server URL. + * + * Servers take ownership synchronously before releasing their listening port, + * so a later server that reuses the same URL cannot be dismissed by the older + * server's asynchronous shutdown. + */ +export function takePresentation( + url: string, +): ExternalPresentation | undefined { + const presentation = activePresentations.get(url); + if (!presentation) return undefined; + activePresentations.delete(url); + return presentation; +} + +/** Dismiss the presentation owned by a server URL, if one exists. */ +export async function dismissPresentation( + url: string, +): Promise { + const presentation = takePresentation(url); + if (!presentation) return { ok: true }; + return presentation.dismiss(); +} diff --git a/packages/shared/test-fixtures/presenter-fixture.mjs b/packages/shared/test-fixtures/presenter-fixture.mjs new file mode 100755 index 000000000..39e9321db --- /dev/null +++ b/packages/shared/test-fixtures/presenter-fixture.mjs @@ -0,0 +1,34 @@ +#!/usr/bin/env node + +import { appendFileSync } from "node:fs"; + +let input = ""; +for await (const chunk of process.stdin) input += chunk; + +const request = JSON.parse(input); +const mode = process.env.PLANNOTATOR_TEST_PRESENTER_MODE; +const logPath = process.env.PLANNOTATOR_TEST_PRESENTER_LOG; +if (logPath) appendFileSync(logPath, `${JSON.stringify(request)}\n`); + +if (mode === "hang") { + setInterval(() => {}, 1_000); +} else if (mode === "flood") { + process.stdout.write("x".repeat(128 * 1024)); +} else if (mode === "invalid") { + process.stdout.write("not json\n"); +} else if (mode === "failure") { + process.stdout.write(JSON.stringify({ + protocol: 1, + ok: false, + error: { code: "fixture_failed", message: "fixture refused" }, + }) + "\n"); + process.exitCode = 1; +} else if (request.action === "present") { + process.stdout.write(JSON.stringify({ + protocol: 1, + ok: true, + handle: { fixture: request.url, kind: request.kind }, + }) + "\n"); +} else { + process.stdout.write(JSON.stringify({ protocol: 1, ok: true }) + "\n"); +} From 04ee91f2b08863a9d23a117acd37f2ea503353d0 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Wed, 29 Jul 2026 10:43:06 -0700 Subject: [PATCH 2/2] fix: bound presenter lifecycle cleanup --- apps/hook/server/annotate-command.test.ts | 49 +++++++++++++++ apps/hook/server/annotate-command.ts | 4 +- apps/hook/server/index.ts | 5 +- apps/hook/server/server-shutdown.test.ts | 47 +++++++++++++++ apps/hook/server/server-shutdown.ts | 47 ++++++++++++--- apps/pi-extension/plannotator-browser.test.ts | 59 +++++++++---------- apps/pi-extension/plannotator-browser.ts | 6 +- packages/shared/presenter.test.ts | 5 +- packages/shared/presenter.ts | 15 +++-- 9 files changed, 189 insertions(+), 48 deletions(-) diff --git a/apps/hook/server/annotate-command.test.ts b/apps/hook/server/annotate-command.test.ts index 235b406a2..530f009f2 100644 --- a/apps/hook/server/annotate-command.test.ts +++ b/apps/hook/server/annotate-command.test.ts @@ -61,6 +61,55 @@ async function runCompletion( } describe("completeAnnotateCommand", () => { + test("waits for server cleanup before publishing or exiting", async () => { + const events: string[] = []; + let finishCleanup!: () => void; + const cleanupFinished = new Promise((resolve) => { + finishCleanup = resolve; + }); + let confirmCleanupStarted!: () => void; + const cleanupStarted = new Promise((resolve) => { + confirmCleanupStarted = resolve; + }); + + const completion = completeAnnotateCommand({ + waitForDecision: async () => { + events.push("decision"); + return { approved: true, feedback: "" }; + }, + settleAfterDecision: async () => { + events.push("settle"); + }, + stopServer: async () => { + events.push("stop:start"); + confirmCleanupStarted(); + await cleanupFinished; + events.push("stop:done"); + }, + requireApproval: false, + emitLegacyOutcome: () => { + events.push("legacy"); + }, + exit: (code) => { + events.push(`exit:${code}`); + }, + }); + + await cleanupStarted; + expect(events).toEqual(["decision", "settle", "stop:start"]); + + finishCleanup(); + await completion; + expect(events).toEqual([ + "decision", + "settle", + "stop:start", + "stop:done", + "legacy", + "exit:0", + ]); + }); + test("publishes approved feedback to matching stdout and result bytes", async () => { const result = await runCompletion( { diff --git a/apps/hook/server/annotate-command.ts b/apps/hook/server/annotate-command.ts index 4b18c2a2a..74e0bd8c3 100644 --- a/apps/hook/server/annotate-command.ts +++ b/apps/hook/server/annotate-command.ts @@ -9,7 +9,7 @@ import { export interface CompleteAnnotateCommandOptions { waitForDecision: () => Promise; settleAfterDecision: () => Promise; - stopServer: () => void; + stopServer: () => void | Promise; requireApproval: boolean; resultFile?: string; writeResultFile?: ( @@ -45,7 +45,7 @@ export async function completeAnnotateCommand({ }: CompleteAnnotateCommandOptions): Promise { const result = await waitForDecision(); await settleAfterDecision(); - stopServer(); + await stopServer(); if (requireApproval || resultFile) { const serialized = serializeStrictAnnotateResult(result); diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index d85490d0b..eed5f5a91 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -85,6 +85,7 @@ import { } from "@plannotator/server/goal-setup"; import { type DiffType, detectManagedVcs, prepareLocalReviewDiff, gitRuntime } from "@plannotator/server/vcs"; import { loadConfig, resolveDefaultDiffType, resolveUseJina, resolveSharingEnabled } from "@plannotator/shared/config"; +import { resolvePresenterCommand } from "@plannotator/shared/presenter"; import { parseReviewArgs } from "@plannotator/shared/review-args"; import { normalizeGoalSetupBundle, @@ -291,8 +292,10 @@ if (isInteractiveNoArgInvocation(args, process.stdin.isTTY)) { // force-exit signal paths. process.on("exit", () => unregisterSession()); +const startupConfig = loadConfig(); const serverShutdown = createServerShutdownCoordinator({ exit: (code) => process.exit(code), + waitForServerCleanup: resolvePresenterCommand(startupConfig) !== undefined, onStopError: (error) => { console.error( `[plannotator] Failed to stop server during shutdown: ${ @@ -313,7 +316,7 @@ process.on("SIGTERM", () => { }); // Check if URL sharing is enabled (default: true) -const sharingEnabled = resolveSharingEnabled(loadConfig()); +const sharingEnabled = resolveSharingEnabled(startupConfig); // Custom share portal URL for self-hosting const shareBaseUrl = process.env.PLANNOTATOR_SHARE_URL || undefined; diff --git a/apps/hook/server/server-shutdown.test.ts b/apps/hook/server/server-shutdown.test.ts index 0df0694f8..d73fa18f4 100644 --- a/apps/hook/server/server-shutdown.test.ts +++ b/apps/hook/server/server-shutdown.test.ts @@ -20,6 +20,7 @@ describe("createServerShutdownCoordinator", () => { exit: (code) => { events.push(`exit:${code}`); }, + waitForServerCleanup: true, }); void coordinator.trackServerStart(Promise.resolve({ @@ -45,6 +46,7 @@ describe("createServerShutdownCoordinator", () => { exit: (code) => { events.push(`exit:${code}`); }, + waitForServerCleanup: true, onStopError: (error) => { events.push(`error:${error instanceof Error ? error.message : error}`); }, @@ -72,6 +74,7 @@ describe("createServerShutdownCoordinator", () => { exit: (code) => { events.push(`exit:${code}`); }, + waitForServerCleanup: true, }); const serverStart = start.promise.then(() => ({ @@ -97,6 +100,7 @@ describe("createServerShutdownCoordinator", () => { exit: (code) => { events.push(`exit:${code}`); }, + waitForServerCleanup: true, }); void coordinator.trackServerStart(Promise.resolve({ @@ -117,4 +121,47 @@ describe("createServerShutdownCoordinator", () => { await gracefulShutdown; expect(events).toEqual(["stop:start", "exit:143", "stop:done"]); }); + + test("exits immediately when no presenter cleanup is needed", async () => { + const events: string[] = []; + const start = createDeferred(); + const coordinator = createServerShutdownCoordinator({ + exit: (code) => { + events.push(`exit:${code}`); + }, + waitForServerCleanup: false, + }); + + void coordinator.trackServerStart(start.promise.then(() => ({ + stop: () => { + events.push("stop"); + }, + }))); + + await coordinator.handleSignal("SIGINT"); + expect(events).toEqual(["exit:130"]); + start.resolve(); + }); + + test("forces exit when graceful cleanup reaches its deadline", async () => { + const events: string[] = []; + const coordinator = createServerShutdownCoordinator({ + exit: (code) => { + events.push(`exit:${code}`); + }, + waitForServerCleanup: true, + cleanupTimeoutMs: 10, + onStopError: (error) => { + events.push(`error:${error instanceof Error ? error.message : error}`); + }, + }); + + void coordinator.trackServerStart(new Promise(() => {})); + await coordinator.handleSignal("SIGTERM"); + + expect(events).toEqual([ + "error:server cleanup timed out after 10ms", + "exit:143", + ]); + }); }); diff --git a/apps/hook/server/server-shutdown.ts b/apps/hook/server/server-shutdown.ts index 3e231bb33..03909b915 100644 --- a/apps/hook/server/server-shutdown.ts +++ b/apps/hook/server/server-shutdown.ts @@ -13,9 +13,13 @@ export interface ServerShutdownCoordinator { export interface ServerShutdownCoordinatorOptions { exit: (code: number) => void; + waitForServerCleanup: boolean; + cleanupTimeoutMs?: number; onStopError?: (error: unknown) => void; } +const DEFAULT_CLEANUP_TIMEOUT_MS = 5_000; + function exitCodeForSignal(signal: FatalSignal): number { return signal === "SIGINT" ? 130 : 143; } @@ -23,12 +27,14 @@ function exitCodeForSignal(signal: FatalSignal): number { /** * Coordinates process signals with the one server owned by the hook CLI. * - * The first signal waits for the active server's idempotent stop routine so - * external presenters are dismissed before process exit. A second signal is - * deliberately treated as a force-exit escape hatch when cleanup is stuck. + * When presenter cleanup is enabled, the first signal gives the active server + * a bounded window to stop. Without a presenter, the first signal preserves + * the CLI's immediate-exit behavior. A second signal always force-exits. */ export function createServerShutdownCoordinator({ exit, + waitForServerCleanup, + cleanupTimeoutMs = DEFAULT_CLEANUP_TIMEOUT_MS, onStopError = () => {}, }: ServerShutdownCoordinatorOptions): ServerShutdownCoordinator { let activeServer: Promise | undefined; @@ -56,14 +62,41 @@ export function createServerShutdownCoordinator({ } shutdownStarted = true; + if (!waitForServerCleanup) { + exit(exitCode); + return; + } + const server = activeServer; + let cleanupTimer: ReturnType | undefined; try { - const startedServer = await server; - await startedServer?.stop(); - } catch (error) { - onStopError(error); + const cleanup = (async () => { + const startedServer = await server; + await startedServer?.stop(); + })(); + const outcome = await Promise.race([ + cleanup.then( + () => ({ status: "completed" as const }), + (error: unknown) => ({ status: "failed" as const, error }), + ), + new Promise<{ status: "timed-out" }>((resolve) => { + cleanupTimer = setTimeout( + () => resolve({ status: "timed-out" }), + cleanupTimeoutMs, + ); + }), + ]); + + if (outcome.status === "failed") { + onStopError(outcome.error); + } else if (outcome.status === "timed-out") { + onStopError( + new Error(`server cleanup timed out after ${cleanupTimeoutMs}ms`), + ); + } } finally { + if (cleanupTimer) clearTimeout(cleanupTimer); // With a real process, the force-exit call above never returns. The // guard also keeps injected test exits from producing a second exit. if (!forceExited) { diff --git a/apps/pi-extension/plannotator-browser.test.ts b/apps/pi-extension/plannotator-browser.test.ts index c82642b63..d60529525 100644 --- a/apps/pi-extension/plannotator-browser.test.ts +++ b/apps/pi-extension/plannotator-browser.test.ts @@ -7,6 +7,7 @@ import { startMarkdownAnnotationSession, startPlanReviewBrowserSession, stopActiveBrowserDecisionSessions, + trackBrowserDecisionSessionStart, shouldUseLocalPrCheckout, } from "./plannotator-browser.ts"; import { loadPlannotatorBrowser } from "./plannotator-browser-runtime.ts"; @@ -322,26 +323,33 @@ describe.skipIf(process.platform === "win32")("Pi presenter lifecycle", () => { }, 10_000); test("shutdown waits for a pending server start and rejects its late session", async () => { - const tempDir = mkdtempSync(join(tmpdir(), "plannotator-pi-pending-start-")); - const logPath = join(tempDir, "requests.jsonl"); - const originalPresenter = process.env.PLANNOTATOR_PRESENTER; - const originalLog = process.env.PLANNOTATOR_TEST_PRESENTER_LOG; - const originalDataDir = process.env.PLANNOTATOR_DATA_DIR; - process.env.PLANNOTATOR_PRESENTER = presenterFixture; - process.env.PLANNOTATOR_TEST_PRESENTER_LOG = logPath; - process.env.PLANNOTATOR_DATA_DIR = join(tempDir, "data"); - + let releaseStart!: () => void; + const startReleased = new Promise((resolve) => { + releaseStart = resolve; + }); + let serverStops = 0; const ctx = { - cwd: tempDir, - hasUI: true, ui: { notify: () => undefined, }, - } as unknown as Parameters[0]; + } as unknown as Parameters[1]; try { await resumePlannotatorBrowserSessions(); - const startup = startPlanReviewBrowserSession(ctx, "# Pending shutdown proof"); + const startup = trackBrowserDecisionSessionStart(async () => { + await startReleased; + return startBrowserDecisionSession( + { + url: "http://localhost:45683", + stop: () => { + serverStops += 1; + }, + }, + ctx, + () => new Promise(() => {}), + "plan", + ); + }); let startupSettled = false; const observedStartup = startup .then( @@ -353,7 +361,9 @@ describe.skipIf(process.platform === "win32")("Pi presenter lifecycle", () => { return outcome; }); - await stopActivePlannotatorBrowserSessions(); + const shutdown = stopActiveBrowserDecisionSessions(); + releaseStart(); + await shutdown; expect(startupSettled).toBe(true); const outcome = await observedStartup; @@ -364,28 +374,13 @@ describe.skipIf(process.platform === "win32")("Pi presenter lifecycle", () => { } expect(outcome.error.message).toContain("shutting down"); } - expect(existsSync(logPath)).toBe(false); + expect(serverStops).toBe(1); } finally { + releaseStart(); await stopActiveBrowserDecisionSessions().catch(() => undefined); await resumePlannotatorBrowserSessions(); - if (originalPresenter === undefined) { - delete process.env.PLANNOTATOR_PRESENTER; - } else { - process.env.PLANNOTATOR_PRESENTER = originalPresenter; - } - if (originalLog === undefined) { - delete process.env.PLANNOTATOR_TEST_PRESENTER_LOG; - } else { - process.env.PLANNOTATOR_TEST_PRESENTER_LOG = originalLog; - } - if (originalDataDir === undefined) { - delete process.env.PLANNOTATOR_DATA_DIR; - } else { - process.env.PLANNOTATOR_DATA_DIR = originalDataDir; - } - rmSync(tempDir, { recursive: true, force: true }); } - }, 10_000); + }); test("the shutdown latch rejects every entrypoint and closes direct late registrations", async () => { const ctx = { diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index fd4b71e91..d981a02b7 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -192,7 +192,11 @@ function createBrowserSessionsShuttingDownError(): Error { return new Error(BROWSER_SESSIONS_SHUTTING_DOWN_MESSAGE); } -function trackBrowserDecisionSessionStart(start: () => Promise): Promise { +/** + * Track a pending browser-session start so shutdown waits for it and rejects + * any session that finishes starting after shutdown has begun. + */ +export function trackBrowserDecisionSessionStart(start: () => Promise): Promise { if (!browserDecisionSessionStartsAllowed) { return Promise.reject(createBrowserSessionsShuttingDownError()); } diff --git a/packages/shared/presenter.test.ts b/packages/shared/presenter.test.ts index 75c9d00b4..f47f10ef4 100644 --- a/packages/shared/presenter.test.ts +++ b/packages/shared/presenter.test.ts @@ -59,11 +59,13 @@ describe("resolvePresenterCommand", () => { describe("presentUrl", () => { test("sends the exact protocol and dismisses the returned handle once", async () => { const requests: PresenterRequest[] = []; + const timeouts: Array = []; const result = await presentUrl("http://localhost:3210", "review", { config: {}, env: { PLANNOTATOR_PRESENTER: "/presenter" }, - invoke: async (_command, request) => { + invoke: async (_command, request, options) => { requests.push(request); + timeouts.push(options.timeoutMs); return request.action === "present" ? { ok: true, @@ -94,6 +96,7 @@ describe("presentUrl", () => { handle: { paneId: "pane-1" }, }, ]); + expect(timeouts).toEqual([15_000, 5_000]); }); test("returns presenter failures as fallback-friendly values", async () => { diff --git a/packages/shared/presenter.ts b/packages/shared/presenter.ts index 4c52b8950..f06b2c077 100644 --- a/packages/shared/presenter.ts +++ b/packages/shared/presenter.ts @@ -8,9 +8,10 @@ import { spawn } from "node:child_process"; import { loadConfig, type PlannotatorConfig } from "./config"; export const PRESENTER_PROTOCOL_VERSION = 1; -// Leave enough time for runtime discovery, Browser startup, and failed-startup -// pane cleanup before the outer process gives up. -export const PRESENTER_TIMEOUT_MS = 60_000; +/** Maximum time allowed for an external presenter to accept a URL. */ +export const PRESENTER_PRESENT_TIMEOUT_MS = 15_000; +/** Maximum time allowed for an external presenter to dismiss its presentation. */ +export const PRESENTER_DISMISS_TIMEOUT_MS = 5_000; export const PRESENTER_MAX_OUTPUT_BYTES = 64 * 1024; const PRESENTER_TERMINATION_GRACE_MS = 10_500; @@ -147,7 +148,11 @@ export async function invokePresenterCommand( request: PresenterRequest, options: PresenterCommandOptions = {}, ): Promise { - const timeoutMs = options.timeoutMs ?? PRESENTER_TIMEOUT_MS; + const timeoutMs = options.timeoutMs ?? ( + request.action === "present" + ? PRESENTER_PRESENT_TIMEOUT_MS + : PRESENTER_DISMISS_TIMEOUT_MS + ); const maxOutputBytes = options.maxOutputBytes ?? PRESENTER_MAX_OUTPUT_BYTES; @@ -311,6 +316,7 @@ export async function presentUrl( }, { signal: options.signal, env, + timeoutMs: PRESENTER_PRESENT_TIMEOUT_MS, }); if (!result.ok) { return { attempted: true, opened: false, error: result.error }; @@ -340,6 +346,7 @@ export async function presentUrl( handle, }, { env, + timeoutMs: PRESENTER_DISMISS_TIMEOUT_MS, }); const result = dismissed.ok ? { ok: true }