refactor: handle issues #147 and #149 (complete helper migration + unit test for ensurePatchloomReadyOrNotify) (#151)

refactor+test: complete migration to ensurePatchloomReadyOrNotify for configureMcp/setup/verifyMcp (#147); improve testability and add unit test for helper (#149)

- Updated remaining commands using the early-ready-notify pattern.
- Moved vscode import inside notify branches (only when UI needed).
- Added ensure test for success path (error paths require VSCode env).
- Re-ran compile + unit tests: all green.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>

Author: SebTardif

src/binary/patchloom.ts

@@ -160,11 +160,16 @@ export function patchloomNeedsUpgrade(status: PatchloomStatus): boolean {
  *
  * This removes duplicated ready-check + notify logic across commands.
  */
-export async function ensurePatchloomReadyOrNotify(contextSuffix = ""): Promise<string | null> {
-  const status = await resolvePatchloomStatus();
-  const vscode = await import("vscode");
+export async function ensurePatchloomReadyOrNotify(
+  contextSuffix = "",
+  testInputs?: PatchloomStatusInputs
+): Promise<string | null> {
+  const status = testInputs
+    ? await resolvePatchloomStatusWithInputs(testInputs)
+    : await resolvePatchloomStatus();
 
   if (!status.ready || !status.binaryPath) {
+    const vscode = await import("vscode");
     const choice = await vscode.window.showWarningMessage(
       `${status.message}${contextSuffix ? `\n\n${contextSuffix}` : ""}`,
       "Open Settings"
@@ -176,6 +181,7 @@ export async function ensurePatchloomReadyOrNotify(contextSuffix = ""): Promise<
   }
 
   if (patchloomNeedsUpgrade(status)) {
+    const vscode = await import("vscode");
     const choice = await vscode.window.showWarningMessage(
       `${status.compatibilityMessage}${contextSuffix ? `\n\n${contextSuffix}` : ""}`,
       "Open Releases"

src/commands/configureMcp.ts

@@ -1,32 +1,14 @@
 import * as fs from "node:fs/promises";
 import * as path from "node:path";
 import * as vscode from "vscode";
-import { patchloomNeedsUpgrade, resolvePatchloomStatus } from "../binary/patchloom.js";
+import { ensurePatchloomReadyOrNotify } from "../binary/patchloom.js";
 import { configureMcpTargets, inspectMcpTargets } from "../mcp/config.js";
 import { activeWorkspaceFolder, describeWorkspaceEnvironment } from "../workspace/readiness.js";
 import { refreshStatusBar } from "../status/statusBar.js";
 
 export async function configureMcp(): Promise<void> {
-  const status = await resolvePatchloomStatus();
-  if (!status.ready) {
-    const choice = await vscode.window.showWarningMessage(
-      `${status.message}\n\nPatchloom needs a working binary before MCP setup can continue.`,
-      "Open Settings"
-    );
-    if (choice === "Open Settings") {
-      await vscode.commands.executeCommand("patchloom.openPatchloomSettings");
-    }
-    return;
-  }
-
-  if (patchloomNeedsUpgrade(status)) {
-    const choice = await vscode.window.showWarningMessage(
-      `${status.compatibilityMessage}\n\nUpgrade Patchloom before MCP setup can continue.`,
-      "Open Releases"
-    );
-    if (choice === "Open Releases") {
-      await vscode.commands.executeCommand("patchloom.openPatchloomReleases");
-    }
+  const binaryPath = await ensurePatchloomReadyOrNotify("Patchloom needs a working binary before MCP setup can continue.");
+  if (!binaryPath) {
     return;
   }
 
@@ -66,7 +48,7 @@ export async function configureMcp(): Promise<void> {
     workspaceFolderPath,
     includeKinds: selectedKinds,
     includeUserTarget: environment.supportsUserMcpConfig,
-    patchloomPathSetting: status.binaryPath,
+    patchloomPathSetting: binaryPath,
     readFile: async (filePath) => {
       try {
         return await fs.readFile(filePath, "utf8");

src/commands/setupWorkspace.ts

@@ -1,34 +1,17 @@
 import * as vscode from "vscode";
-import { PATCHLOOM_DOCS_URL, PATCHLOOM_RELEASES_URL, patchloomNeedsUpgrade, resolvePatchloomStatus } from "../binary/patchloom.js";
+import { ensurePatchloomReadyOrNotify, PATCHLOOM_DOCS_URL, PATCHLOOM_RELEASES_URL } from "../binary/patchloom.js";
 import { describeWorkspaceEnvironment, inspectWorkspaceReadiness } from "../workspace/readiness.js";
 
 export async function setupWorkspace(): Promise<void> {
-  const status = await resolvePatchloomStatus();
-  if (!status.ready) {
-    const choice = await vscode.window.showWarningMessage(
-      `${status.message}\n\nPatchloom needs a working binary before workspace setup can continue.`,
-      "Open Settings"
-    );
-    if (choice === "Open Settings") {
-      await vscode.commands.executeCommand("patchloom.openPatchloomSettings");
-    }
+  const binaryPath = await ensurePatchloomReadyOrNotify("Patchloom needs a working binary before workspace setup can continue.");
+  if (!binaryPath) {
     return;
   }
 
   const readiness = await inspectWorkspaceReadiness({
     promptIfMany: true,
     placeHolder: "Select the workspace folder to inspect for Patchloom setup"
   });
-  if (patchloomNeedsUpgrade(status)) {
-    const choice = await vscode.window.showWarningMessage(
-      `${status.compatibilityMessage}\n\nUpgrade Patchloom before workspace setup can continue.`,
-      "Open Releases"
-    );
-    if (choice === "Open Releases") {
-      await vscode.commands.executeCommand("patchloom.openPatchloomReleases");
-    }
-    return;
-  }
 
   if (!readiness.hasWorkspace) {
     await vscode.window.showWarningMessage("Open a workspace folder before running Patchloom: Setup Workspace.");

src/commands/verifyMcp.ts

@@ -1,5 +1,5 @@
 import { spawn } from "node:child_process";
-import { patchloomNeedsUpgrade, resolvePatchloomStatus } from "../binary/patchloom.js";
+import { ensurePatchloomReadyOrNotify } from "../binary/patchloom.js";
 import { getPatchloomLog } from "../logging/outputChannel.js";
 
 export interface VerifyMcpInputs {
@@ -16,16 +16,8 @@ export interface VerifyMcpResult {
 
 export async function verifyMcp(): Promise<void> {
   const vscode = await import("vscode");
-  const status = await resolvePatchloomStatus();
-  if (!status.ready || !status.binaryPath) {
-    await vscode.window.showWarningMessage(status.message);
-    return;
-  }
-
-  if (patchloomNeedsUpgrade(status)) {
-    await vscode.window.showWarningMessage(
-      `${status.compatibilityMessage}\n\nUpgrade Patchloom before verifying the MCP server.`
-    );
+  const binaryPath = await ensurePatchloomReadyOrNotify("Upgrade Patchloom before verifying the MCP server.");
+  if (!binaryPath) {
     return;
   }
 
@@ -37,7 +29,7 @@ export async function verifyMcp(): Promise<void> {
     },
     async (progress) => {
       progress.report({ message: "Verifying MCP server..." });
-      return verifyMcpServer({ binaryPath: status.binaryPath! });
+      return verifyMcpServer({ binaryPath });
     }
   );
 

test/unit/binary.test.ts

@@ -13,6 +13,7 @@ import {
   MINIMUM_SUPPORTED_PATCHLOOM_VERSION,
   patchloomNeedsUpgrade,
   parsePatchloomVersion,
+  ensurePatchloomReadyOrNotify,
   resolvePatchloomStatusWithInputs
 } from "../../src/binary/patchloom.js";
 import {
@@ -127,6 +128,20 @@ test("resolvePatchloomStatusWithInputs exposes compatibility diagnostics", async
   assert.match(status.compatibilityMessage ?? "", /older than the minimum supported version/i);
 });
 
+test("ensurePatchloomReadyOrNotify returns path for ready supported status (test inputs)", async () => {
+  const path = await ensurePatchloomReadyOrNotify("", {
+    configuredPath: "/good/patchloom",
+    canExecute: async () => true,
+    getVersion: async () => "patchloom 0.2.0"
+  });
+  assert.equal(path, "/good/patchloom");
+});
+
+// Note: error paths (not-ready, upgrade) execute vscode.window.show* which is
+// not unit-testable in pure node (would require full VS Code test env or mocks).
+// They are exercised via command integration and manual. The core logic delegates
+// to resolve+needsUpgrade which are unit tested.
+
 test("defaultWorkspaceFolderIndex prefers active folders and only auto-selects single roots", () => {
   assert.equal(defaultWorkspaceFolderIndex(3, 2), 2);
   assert.equal(defaultWorkspaceFolderIndex(1, undefined), 0);