fix(cli): upgrade patches repo-local node_modules; doctor verifies two-layer commit hooks

.changeset/fix-1190-esm-patch-doctor-commit-hooks.md

@@ -0,0 +1,5 @@
+---
+"@bradygaster/squad-cli": patch
+---
+
+Fix two upgrade-pipeline gaps from #1190: the postinstall ESM patcher now patches every search root instead of stopping at the first already-patched copy, and `squad upgrade` re-runs it against the repo-local node_modules so globally-installed CLIs no longer leave the consumer repo's vscode-jsonrpc/copilot-sdk unpatched. `squad doctor` now also verifies the pre-commit/post-commit hooks required by the two-layer/orphan state backends, instead of only the four sync hooks.

packages/squad-cli/scripts/patch-esm-imports.mjs

@@ -18,7 +18,7 @@
 
 import { readFileSync, writeFileSync, existsSync } from 'fs';
 import { join, dirname } from 'path';
-import { fileURLToPath } from 'url';
+import { fileURLToPath, pathToFileURL } from 'url';
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
 
@@ -36,16 +36,21 @@ const SEARCH_ROOTS = [
  * This is the canonical fix: once the package has proper exports, Node's
  * ESM resolver handles every subpath ('vscode-jsonrpc/node', '/browser', etc.)
  * without needing per-file patches.
+ *
+ * Patches EVERY root that contains a copy, not just the first one found:
+ * stopping at the first (already-patched) copy left repo-local node_modules
+ * unpatched on global installs (#1190).
  */
-function patchVscodeJsonrpcExports() {
+export function patchVscodeJsonrpcExports(searchRoots = SEARCH_ROOTS) {
   const exportsField = {
     '.': { types: './lib/common/api.d.ts', default: './lib/node/main.js' },
     './node': { node: './lib/node/main.js', types: './lib/node/main.d.ts' },
     './node.js': { node: './lib/node/main.js', types: './lib/node/main.d.ts' },
     './browser': { types: './lib/browser/main.d.ts', browser: './lib/browser/main.js' },
   };
 
-  for (const root of SEARCH_ROOTS) {
+  let patchedAny = false;
+  for (const root of searchRoots) {
     const pkgPath = join(root, 'vscode-jsonrpc', 'package.json');
     if (!existsSync(pkgPath)) continue;
 
@@ -54,29 +59,30 @@ function patchVscodeJsonrpcExports() {
       const pkg = JSON.parse(raw);
 
       if (pkg.exports && pkg.exports['./node.js']) {
-        console.log('⏭️  vscode-jsonrpc already has complete exports field — skipping');
-        return false;
+        console.log(`⏭️  vscode-jsonrpc already has complete exports field — skipping (${pkgPath})`);
+        continue;
       }
 
       pkg.exports = exportsField;
       writeFileSync(pkgPath, JSON.stringify(pkg, null, 2) + '\n', 'utf8');
-      console.log('✅ Patched vscode-jsonrpc/package.json with exports field (Node 22/24+ ESM fix)');
-      return true;
+      console.log(`✅ Patched vscode-jsonrpc/package.json with exports field (${pkgPath})`);
+      patchedAny = true;
     } catch (err) {
       console.warn('⚠️  Failed to patch vscode-jsonrpc exports:', err.message);
-      return false;
     }
   }
 
-  return false;
+  return patchedAny;
 }
 
 /**
  * Layer 2 — Patch copilot-sdk session.js import (defense-in-depth).
  * Rewrites extensionless 'vscode-jsonrpc/node' to 'vscode-jsonrpc/node.js'.
+ * Same all-roots semantics as Layer 1 (#1190).
  */
-function patchCopilotSdkSessionJs() {
-  for (const root of SEARCH_ROOTS) {
+export function patchCopilotSdkSessionJs(searchRoots = SEARCH_ROOTS) {
+  let patchedAny = false;
+  for (const root of searchRoots) {
     const sessionJsPath = join(root, '@github', 'copilot-sdk', 'dist', 'session.js');
     if (!existsSync(sessionJsPath)) continue;
 
@@ -90,19 +96,20 @@ function patchCopilotSdkSessionJs() {
 
       if (patched !== content) {
         writeFileSync(sessionJsPath, patched, 'utf8');
-        console.log('✅ Patched @github/copilot-sdk session.js ESM imports');
-        return true;
+        console.log(`✅ Patched @github/copilot-sdk session.js ESM imports (${sessionJsPath})`);
+        patchedAny = true;
       }
-      return false;
     } catch (err) {
       console.warn('⚠️  Failed to patch copilot-sdk session.js:', err.message);
-      return false;
     }
   }
 
-  return false;
+  return patchedAny;
 }
 
-// Run both layers
-patchVscodeJsonrpcExports();
-patchCopilotSdkSessionJs();
+// Run both layers when executed directly (postinstall). `squad upgrade` imports
+// the functions above and points them at the consumer repo's node_modules (#1190).
+if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) {
+  patchVscodeJsonrpcExports();
+  patchCopilotSdkSessionJs();
+}

packages/squad-cli/src/cli/commands/doctor.ts

@@ -516,7 +516,9 @@ function checkCopilotCli(): Promise<DoctorCheck> {
 // ── git sync hooks check ─────────────────────────────────────────────
 
 const SQUAD_SYNC_HOOK_MARKER = '# --- squad-sync-hook ---';
-const REQUIRED_SYNC_HOOKS = ['pre-push', 'post-merge', 'post-rewrite', 'post-checkout'] as const;
+// Must match the full set installed by install-hooks.ts: the four sync hooks
+// plus pre-commit/post-commit, which guard and flush two-layer state (#1190).
+const REQUIRED_SYNC_HOOKS = ['pre-push', 'post-merge', 'post-rewrite', 'post-checkout', 'pre-commit', 'post-commit'] as const;
 
 /**
  * Check that squad git sync hooks are installed when the state backend requires them.

packages/squad-cli/src/cli/core/upgrade.ts

@@ -6,6 +6,7 @@
 
 import path from 'node:path';
 import { execFileSync } from 'node:child_process';
+import { fileURLToPath, pathToFileURL } from 'node:url';
 import { FSStorageProvider } from '@bradygaster/squad-sdk';
 import { success, warn, info, dim, bold } from './output.js';
 import { fatal } from './errors.js';
@@ -768,6 +769,49 @@ function refreshSquadTemplatesDir(dest: string, templatesDir: string): void {
   }
 }
 
+/**
+ * Re-run the ESM import patcher against the project's own node_modules (#1190).
+ *
+ * npm runs postinstall with cwd inside the installed package dir, so a global
+ * `npm install -g` only ever patches the global copy — the consumer repo's
+ * node_modules stays unpatched and `squad doctor` keeps failing its
+ * vscode-jsonrpc / copilot-sdk checks there. Loading the patch script and
+ * pointing it at `<dest>/node_modules` closes that gap on every upgrade.
+ *
+ * Returns true when at least one file was patched.
+ */
+export async function ensureEsmImportsPatched(dest: string): Promise<boolean> {
+  // Locate scripts/patch-esm-imports.mjs by walking up from the compiled file,
+  // same approach as getTemplatesDir() — works from dist/cli/core/ and bundles.
+  let dir = path.dirname(fileURLToPath(import.meta.url));
+  let scriptPath: string | undefined;
+  for (let i = 0; i < 6; i++) {
+    const candidate = path.join(dir, 'scripts', 'patch-esm-imports.mjs');
+    if (storage.existsSync(candidate)) {
+      scriptPath = candidate;
+      break;
+    }
+    const parent = path.dirname(dir);
+    if (parent === dir) break;
+    dir = parent;
+  }
+  if (!scriptPath) return false;
+
+  try {
+    const patcher = await import(pathToFileURL(scriptPath).href) as {
+      patchVscodeJsonrpcExports: (searchRoots?: string[]) => boolean;
+      patchCopilotSdkSessionJs: (searchRoots?: string[]) => boolean;
+    };
+    const roots = [path.join(dest, 'node_modules')];
+    const patchedExports = patcher.patchVscodeJsonrpcExports(roots);
+    const patchedSession = patcher.patchCopilotSdkSessionJs(roots);
+    return patchedExports || patchedSession;
+  } catch (err) {
+    warn(`Could not patch ESM imports in ${path.join(dest, 'node_modules')}: ${err instanceof Error ? err.message : err}`);
+    return false;
+  }
+}
+
 /**
  * Run all ensure* checks and skill/template sync — shared by both code paths
  */
@@ -853,6 +897,13 @@ async function runEnsureChecks(dest: string, templatesDir: string, filesUpdated:
     success(`removed stale squad_state from ${tomb.path} (now lives in .mcp.json)`);
     filesUpdated.push('.copilot/mcp-config.json (tombstoned)');
   }
+
+  // #1190: patch ESM imports in the repo-local node_modules — postinstall only
+  // ever runs against the installed package's own directory tree.
+  if (await ensureEsmImportsPatched(dest)) {
+    success('patched ESM imports in repo-local node_modules (vscode-jsonrpc / copilot-sdk, see #449)');
+    filesUpdated.push('node_modules (ESM patch)');
+  }
 }
 
 /** Human-readable single-line description of an McpSpec for success() messages. */

test/cli/doctor.test.ts

@@ -356,7 +356,7 @@ describe('squad doctor', () => {
     await writeFile(join(squadDir, 'config.json'), JSON.stringify({ stateBackend: 'two-layer' }));
     const hooksDir = join(TEST_ROOT, '.git', 'hooks');
     await mkdir(hooksDir, { recursive: true });
-    for (const hookName of ['pre-push', 'post-merge', 'post-rewrite', 'post-checkout']) {
+    for (const hookName of ['pre-push', 'post-merge', 'post-rewrite', 'post-checkout', 'pre-commit', 'post-commit']) {
       await writeFile(
         join(hooksDir, hookName),
         `#!/bin/sh\n# --- squad-sync-hook ---\n# squad sync hook\n`,
@@ -368,6 +368,30 @@ describe('squad doctor', () => {
     expect(result?.message).toContain('two-layer');
   });
 
+  it('reports FAIL when stateBackend=two-layer has sync hooks but no pre-commit/post-commit (#1190)', async () => {
+    const squadDir = join(TEST_ROOT, '.squad');
+    await mkdir(squadDir, { recursive: true });
+    execFileSync('git', ['init', '--quiet', '-b', 'main'], { cwd: TEST_ROOT });
+    await writeFile(join(squadDir, 'config.json'), JSON.stringify({ stateBackend: 'two-layer' }));
+    const hooksDir = join(TEST_ROOT, '.git', 'hooks');
+    await mkdir(hooksDir, { recursive: true });
+    // The #1185 upgrade path installed only the four sync hooks — the commit
+    // hooks that actually write to the squad-state branch were never installed.
+    for (const hookName of ['pre-push', 'post-merge', 'post-rewrite', 'post-checkout']) {
+      await writeFile(
+        join(hooksDir, hookName),
+        `#!/bin/sh\n# --- squad-sync-hook ---\n# squad sync hook\n`,
+      );
+    }
+
+    const result = checkGitSyncHooks(TEST_ROOT, squadDir);
+    expect(result).toBeDefined();
+    expect(result?.status).toBe('fail');
+    expect(result?.message).toContain('pre-commit');
+    expect(result?.message).toContain('post-commit');
+    expect(result?.message).toContain('squad install-hooks');
+  });
+
   it.each(['two-layer', 'orphan', 'git-notes'] as const)('reports PASS when stateBackend=%s has decisions.md on squad-state only', async (stateBackend) => {
     await scaffold(TEST_ROOT);
     execFileSync('git', ['init', '--quiet', '-b', 'main'], { cwd: TEST_ROOT });
@@ -437,7 +461,7 @@ describe('checkGitSyncHooks — git rev-parse --git-dir resolution', () => {
     // Install squad hooks in the actual .git/hooks dir (same as git rev-parse --git-dir → '.git')
     const hooksDir = join(repoDir, '.git', 'hooks');
     await mkdir(hooksDir, { recursive: true });
-    for (const hookName of ['pre-push', 'post-merge', 'post-rewrite', 'post-checkout']) {
+    for (const hookName of ['pre-push', 'post-merge', 'post-rewrite', 'post-checkout', 'pre-commit', 'post-commit']) {
       await writeFile(
         join(hooksDir, hookName),
         `#!/bin/sh\n# --- squad-sync-hook ---\n# squad sync hook\n`,