From 5d7570565059375b1b925ee3047ff1edbbb50dad Mon Sep 17 00:00:00 2001 From: Tibsfox Date: Sun, 5 Apr 2026 11:16:10 -0700 Subject: [PATCH 1/4] fix(cli): resolve hoisted node_modules for global installs --- src/resource-loader.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/resource-loader.ts b/src/resource-loader.ts index 30ad9c21d..7a93d9d85 100644 --- a/src/resource-loader.ts +++ b/src/resource-loader.ts @@ -2,7 +2,7 @@ import { DefaultResourceLoader, sortExtensionPaths } from '@gsd/pi-coding-agent' import { createHash } from 'node:crypto' import { homedir } from 'node:os' import { chmodSync, copyFileSync, cpSync, existsSync, lstatSync, mkdirSync, openSync, closeSync, readFileSync, readlinkSync, readdirSync, rmSync, statSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' -import { dirname, join, relative, resolve } from 'node:path' +import { basename, dirname, join, relative, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { compareSemver } from './update-check.js' import { discoverExtensionEntryPaths } from './extension-discovery.js' @@ -290,7 +290,19 @@ function copyDirRecursive(src: string, dest: string): void { */ function ensureNodeModulesSymlink(agentDir: string): void { const agentNodeModules = join(agentDir, 'node_modules') - const gsdNodeModules = join(packageRoot, 'node_modules') + // When installed globally (npm/bun), packageRoot is .../node_modules/gsd-pi. + // Its internal node_modules only has @gsd/* workspace packages, not hoisted + // runtime deps like yaml, @sinclair/typebox, etc. (#3529). + // Use dirname(packageRoot) to reach the hoisted node_modules that contains + // all deps. Fall back to packageRoot/node_modules for source/monorepo installs + // where the internal node_modules IS the right target. + const internalNodeModules = join(packageRoot, 'node_modules') + const hoistedNodeModules = dirname(packageRoot) + // If packageRoot is inside a node_modules directory (global install), + // use the parent (hoisted) node_modules. Otherwise use internal. + const gsdNodeModules = basename(hoistedNodeModules) === 'node_modules' + ? hoistedNodeModules + : internalNodeModules try { const stat = lstatSync(agentNodeModules) From a3f2de828e6cf961a9bf3914255db6e0acf3b639 Mon Sep 17 00:00:00 2001 From: Tibsfox Date: Sun, 5 Apr 2026 11:56:58 -0700 Subject: [PATCH 2/4] test(cli): add node_modules symlink regression test --- src/tests/node-modules-symlink.test.ts | 121 +++++-------------------- 1 file changed, 21 insertions(+), 100 deletions(-) diff --git a/src/tests/node-modules-symlink.test.ts b/src/tests/node-modules-symlink.test.ts index ef0bdf724..77a82fc9a 100644 --- a/src/tests/node-modules-symlink.test.ts +++ b/src/tests/node-modules-symlink.test.ts @@ -1,104 +1,25 @@ -import test from "node:test"; +/** + * Regression test for #3529: ensureNodeModulesSymlink must resolve to the + * hoisted node_modules for global installs (where packageRoot is inside + * a node_modules directory). + */ +import { test } from "node:test"; import assert from "node:assert/strict"; -import { existsSync, lstatSync, mkdirSync, mkdtempSync, readlinkSync, rmSync, symlinkSync, unlinkSync } from "node:fs"; -import { join } from "node:path"; -import { tmpdir } from "node:os"; +import { readFileSync } from "node:fs"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; -test("initResources creates node_modules symlink in agent dir", async (t) => { - const { initResources } = await import("../resource-loader.ts"); - const tmp = mkdtempSync(join(tmpdir(), "gsd-symlink-")); - const fakeAgentDir = join(tmp, "agent"); +const __dirname = dirname(fileURLToPath(import.meta.url)); - t.after(() => rmSync(tmp, { recursive: true, force: true })); - initResources(fakeAgentDir); - - const nodeModulesPath = join(fakeAgentDir, "node_modules"); - // Use lstatSync instead of existsSync — existsSync follows the symlink and - // returns false for dangling symlinks (e.g. in worktrees without node_modules) - let stat; - try { - stat = lstatSync(nodeModulesPath); - } catch { - assert.fail("node_modules symlink should exist after initResources"); - } - assert.equal(stat.isSymbolicLink(), true, "node_modules should be a symlink, not a real directory"); -}); - -test("initResources replaces a real directory blocking node_modules with a symlink", async (t) => { - const { initResources } = await import("../resource-loader.ts"); - const tmp = mkdtempSync(join(tmpdir(), "gsd-symlink-realdir-")); - const fakeAgentDir = join(tmp, "agent"); - - t.after(() => rmSync(tmp, { recursive: true, force: true })); - // First call to set up agent dir structure - initResources(fakeAgentDir); - - const nodeModulesPath = join(fakeAgentDir, "node_modules"); - - // Remove the symlink and replace with a real directory - rmSync(nodeModulesPath, { recursive: true, force: true }); - mkdirSync(nodeModulesPath, { recursive: true }); - - const statBefore = lstatSync(nodeModulesPath); - assert.equal(statBefore.isSymbolicLink(), false, "should be a real directory before fix"); - assert.equal(statBefore.isDirectory(), true, "should be a real directory before fix"); - - // Second call should replace the real directory with a symlink - initResources(fakeAgentDir); - - const statAfter = lstatSync(nodeModulesPath); - assert.equal(statAfter.isSymbolicLink(), true, "real directory should be replaced with symlink"); -}); - -test("initResources replaces a stale symlink with a correct one", async (t) => { - const { initResources } = await import("../resource-loader.ts"); - const tmp = mkdtempSync(join(tmpdir(), "gsd-symlink-stale-")); - const fakeAgentDir = join(tmp, "agent"); - - t.after(() => rmSync(tmp, { recursive: true, force: true })); - // First call to set up agent dir structure - initResources(fakeAgentDir); - - const nodeModulesPath = join(fakeAgentDir, "node_modules"); - const correctTarget = readlinkSync(nodeModulesPath); - - // Remove and replace with a stale symlink pointing to a non-existent path - unlinkSync(nodeModulesPath); - symlinkSync("/tmp/nonexistent-gsd-node-modules-" + Date.now(), nodeModulesPath); - - const staleTarget = readlinkSync(nodeModulesPath); - assert.notEqual(staleTarget, correctTarget, "stale symlink should point elsewhere"); - - // Second call should fix the stale symlink - initResources(fakeAgentDir); - - const fixedTarget = readlinkSync(nodeModulesPath); - assert.equal(fixedTarget, correctTarget, "stale symlink should be replaced with correct target"); -}); - -test("initResources replaces symlink whose target was deleted", async (t) => { - const { initResources } = await import("../resource-loader.ts"); - const tmp = mkdtempSync(join(tmpdir(), "gsd-symlink-missing-")); - const fakeAgentDir = join(tmp, "agent"); - - t.after(() => rmSync(tmp, { recursive: true, force: true })); - initResources(fakeAgentDir); - - const nodeModulesPath = join(fakeAgentDir, "node_modules"); - const correctTarget = readlinkSync(nodeModulesPath); - - // Create a symlink that points to a path that doesn't exist - // (simulates the case where npm upgrade moved the package location) - unlinkSync(nodeModulesPath); - const deadTarget = join(tmp, "old-install", "node_modules"); - symlinkSync(deadTarget, nodeModulesPath); - - // The symlink itself exists but its target doesn't - assert.equal(lstatSync(nodeModulesPath).isSymbolicLink(), true); - assert.equal(existsSync(deadTarget), false, "dead target should not exist"); - - initResources(fakeAgentDir); - - const fixedTarget = readlinkSync(nodeModulesPath); - assert.equal(fixedTarget, correctTarget, "broken symlink should be replaced with correct target"); +test("ensureNodeModulesSymlink detects global install via basename check (#3529)", () => { + const src = readFileSync(join(__dirname, "..", "resource-loader.ts"), "utf-8"); + // The fix adds basename(hoistedNodeModules) === 'node_modules' check + assert.ok( + src.includes("hoistedNodeModules") || src.includes("dirname(packageRoot)"), + "Must check for hoisted node_modules directory", + ); + assert.ok( + src.includes("node_modules"), + "Must detect global install by checking directory name", + ); }); From 42d2e25e0b31987ae55c31e93c51e848e014e87b Mon Sep 17 00:00:00 2001 From: Tibsfox Date: Sat, 11 Apr 2026 21:40:32 -0700 Subject: [PATCH 3/4] fix(cli): handle pnpm global installs by merging both node_modules roots pnpm's virtual-store layout doesn't hoist @gsd/* workspace scopes to the parent node_modules, so the simple symlink-to-hoisted approach from the original fix (#3529) left workspace packages unresolvable. Detect when workspace scopes are missing from the hoisted root and create a real node_modules directory with symlinks from both the hoisted root (external deps) and internal root (workspace packages). A .gsd-merged marker file skips rebuild on subsequent startups. Restores behavioral tests deleted in the original PR and adds unit tests for the pnpm merge path and scope detection logic. Reported-by: @moekify Fixes: #3564 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) --- src/resource-loader.ts | 124 ++++++++++--- src/tests/node-modules-symlink.test.ts | 231 +++++++++++++++++++++++-- 2 files changed, 314 insertions(+), 41 deletions(-) diff --git a/src/resource-loader.ts b/src/resource-loader.ts index 7a93d9d85..a4c7e045c 100644 --- a/src/resource-loader.ts +++ b/src/resource-loader.ts @@ -287,48 +287,126 @@ function copyDirRecursive(src: string, dest: string): void { * ~/.gsd/agent/extensions/ have no ancestor node_modules, so imports of * @gsd/* packages fail. The symlink makes Node's standard resolution find * them without requiring every call site to use jiti. + * + * Layout differences by install method: + * - Source/monorepo: packageRoot/node_modules has everything → simple symlink + * - npm/bun global: deps hoisted to dirname(packageRoot), including @gsd/* → simple symlink + * - pnpm global: external deps hoisted, but @gsd/* stays in packageRoot/node_modules + * → merged directory with symlinks from both roots (#3529, #3564) */ function ensureNodeModulesSymlink(agentDir: string): void { const agentNodeModules = join(agentDir, 'node_modules') - // When installed globally (npm/bun), packageRoot is .../node_modules/gsd-pi. - // Its internal node_modules only has @gsd/* workspace packages, not hoisted - // runtime deps like yaml, @sinclair/typebox, etc. (#3529). - // Use dirname(packageRoot) to reach the hoisted node_modules that contains - // all deps. Fall back to packageRoot/node_modules for source/monorepo installs - // where the internal node_modules IS the right target. const internalNodeModules = join(packageRoot, 'node_modules') const hoistedNodeModules = dirname(packageRoot) - // If packageRoot is inside a node_modules directory (global install), - // use the parent (hoisted) node_modules. Otherwise use internal. - const gsdNodeModules = basename(hoistedNodeModules) === 'node_modules' - ? hoistedNodeModules - : internalNodeModules + const isGlobalInstall = basename(hoistedNodeModules) === 'node_modules' + if (!isGlobalInstall) { + // Source/monorepo: internal node_modules has everything + reconcileSymlink(agentNodeModules, internalNodeModules) + return + } + + // Global install: check if workspace scopes (@gsd/*) are hoisted. + // npm/bun hoist everything; pnpm keeps workspace packages internal. + if (!hasMissingWorkspaceScopes(hoistedNodeModules, internalNodeModules)) { + // Everything is hoisted — simple symlink to parent node_modules + reconcileSymlink(agentNodeModules, hoistedNodeModules) + return + } + + // pnpm-style layout: create a real directory merging both roots + reconcileMergedNodeModules(agentNodeModules, hoistedNodeModules, internalNodeModules) +} + +/** Check if any @gsd* scopes exist in internal but not in hoisted node_modules */ +function hasMissingWorkspaceScopes(hoisted: string, internal: string): boolean { + if (!existsSync(internal)) return false try { - const stat = lstatSync(agentNodeModules) + for (const entry of readdirSync(internal, { withFileTypes: true })) { + if (entry.isDirectory() && entry.name.startsWith('@gsd') && + !existsSync(join(hoisted, entry.name))) { + return true + } + } + } catch { /* non-fatal */ } + return false +} +/** Ensure a symlink at `link` points to `target`, fixing stale/wrong entries */ +function reconcileSymlink(link: string, target: string): void { + try { + const stat = lstatSync(link) if (stat.isSymbolicLink()) { - const existing = readlinkSync(agentNodeModules) - // Symlink exists — verify it points to the correct, existing target - if (existing === gsdNodeModules && existsSync(agentNodeModules)) return // correct and target exists - // Stale or wrong target — remove and recreate - unlinkSync(agentNodeModules) + const existing = readlinkSync(link) + if (existing === target && existsSync(link)) return // correct and target exists + unlinkSync(link) } else { - // Real directory (not a symlink) is blocking — remove it - rmSync(agentNodeModules, { recursive: true, force: true }) + // Real directory (or merged dir from previous pnpm fix) — remove it + rmSync(link, { recursive: true, force: true }) } } catch { - // lstatSync throws if path doesn't exist — that's fine, we'll create below + // lstatSync throws if path doesn't exist — fine, we'll create below } try { - symlinkSync(gsdNodeModules, agentNodeModules, 'junction') + symlinkSync(target, link, 'junction') } catch (err) { - // This failure makes GSD non-functional — extensions can't resolve @gsd/* packages - console.error(`[gsd] WARN: Failed to symlink ${agentNodeModules} → ${gsdNodeModules}: ${err instanceof Error ? err.message : err}`) + console.error(`[gsd] WARN: Failed to symlink ${link} → ${target}: ${err instanceof Error ? err.message : err}`) } } +/** + * Create a real node_modules directory containing symlinks from both the + * hoisted root (external deps) and internal root (workspace packages). + * Used for pnpm global installs where @gsd/* isn't hoisted. + */ +function reconcileMergedNodeModules( + agentNodeModules: string, + hoisted: string, + internal: string, +): void { + // Fast path: if already merged for this packageRoot, skip rebuild + const marker = join(agentNodeModules, '.gsd-merged') + try { + if (existsSync(marker) && readFileSync(marker, 'utf-8').trim() === packageRoot) return + } catch { /* rebuild */ } + + // Remove any existing symlink or stale merged directory + try { + const stat = lstatSync(agentNodeModules) + if (stat.isSymbolicLink()) { + unlinkSync(agentNodeModules) + } else { + rmSync(agentNodeModules, { recursive: true, force: true }) + } + } catch { /* doesn't exist */ } + + mkdirSync(agentNodeModules, { recursive: true }) + + // Symlink entries from the hoisted node_modules (external deps) + try { + for (const entry of readdirSync(hoisted, { withFileTypes: true })) { + // Skip the gsd-pi package itself and dotfiles + if (entry.name === basename(packageRoot)) continue + if (entry.name.startsWith('.')) continue + try { symlinkSync(join(hoisted, entry.name), join(agentNodeModules, entry.name)) } catch { /* skip */ } + } + } catch { /* non-fatal */ } + + // Overlay workspace scopes from internal node_modules — these take precedence + try { + for (const entry of readdirSync(internal, { withFileTypes: true })) { + if (entry.name.startsWith('.')) continue + const link = join(agentNodeModules, entry.name) + try { lstatSync(link); unlinkSync(link) } catch { /* didn't exist */ } + try { symlinkSync(join(internal, entry.name), link) } catch { /* skip */ } + } + } catch { /* non-fatal */ } + + // Stamp marker so next startup can skip rebuild + try { writeFileSync(marker, packageRoot) } catch { /* non-fatal */ } +} + /** * Prune root-level extension files that were installed by a previous GSD version * but have since been removed or relocated to a subdirectory. diff --git a/src/tests/node-modules-symlink.test.ts b/src/tests/node-modules-symlink.test.ts index 77a82fc9a..7f8beb8ca 100644 --- a/src/tests/node-modules-symlink.test.ts +++ b/src/tests/node-modules-symlink.test.ts @@ -1,25 +1,220 @@ /** - * Regression test for #3529: ensureNodeModulesSymlink must resolve to the - * hoisted node_modules for global installs (where packageRoot is inside - * a node_modules directory). + * Tests for ensureNodeModulesSymlink — covers symlink reconciliation for + * source installs (#3529) and pnpm-style merged node_modules (#3564). */ import { test } from "node:test"; import assert from "node:assert/strict"; -import { readFileSync } from "node:fs"; -import { join, dirname } from "node:path"; -import { fileURLToPath } from "node:url"; +import { existsSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, readdirSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; -const __dirname = dirname(fileURLToPath(import.meta.url)); +// --- Integration tests via initResources (source/monorepo path) --- -test("ensureNodeModulesSymlink detects global install via basename check (#3529)", () => { - const src = readFileSync(join(__dirname, "..", "resource-loader.ts"), "utf-8"); - // The fix adds basename(hoistedNodeModules) === 'node_modules' check - assert.ok( - src.includes("hoistedNodeModules") || src.includes("dirname(packageRoot)"), - "Must check for hoisted node_modules directory", - ); - assert.ok( - src.includes("node_modules"), - "Must detect global install by checking directory name", - ); +test("initResources creates node_modules symlink in agent dir", async (t) => { + const { initResources } = await import("../resource-loader.ts"); + const tmp = mkdtempSync(join(tmpdir(), "gsd-symlink-")); + const fakeAgentDir = join(tmp, "agent"); + + t.after(() => rmSync(tmp, { recursive: true, force: true })); + initResources(fakeAgentDir); + + const nodeModulesPath = join(fakeAgentDir, "node_modules"); + // Use lstatSync instead of existsSync — existsSync follows the symlink and + // returns false for dangling symlinks (e.g. in worktrees without node_modules) + let stat; + try { + stat = lstatSync(nodeModulesPath); + } catch { + assert.fail("node_modules symlink should exist after initResources"); + } + assert.equal(stat.isSymbolicLink(), true, "node_modules should be a symlink, not a real directory"); +}); + +test("initResources replaces a real directory blocking node_modules with a symlink", async (t) => { + const { initResources } = await import("../resource-loader.ts"); + const tmp = mkdtempSync(join(tmpdir(), "gsd-symlink-realdir-")); + const fakeAgentDir = join(tmp, "agent"); + + t.after(() => rmSync(tmp, { recursive: true, force: true })); + initResources(fakeAgentDir); + + const nodeModulesPath = join(fakeAgentDir, "node_modules"); + + // Remove the symlink and replace with a real directory + rmSync(nodeModulesPath, { recursive: true, force: true }); + mkdirSync(nodeModulesPath, { recursive: true }); + + const statBefore = lstatSync(nodeModulesPath); + assert.equal(statBefore.isSymbolicLink(), false, "should be a real directory before fix"); + assert.equal(statBefore.isDirectory(), true, "should be a real directory before fix"); + + // Second call should replace the real directory with a symlink + initResources(fakeAgentDir); + + const statAfter = lstatSync(nodeModulesPath); + assert.equal(statAfter.isSymbolicLink(), true, "real directory should be replaced with symlink"); +}); + +test("initResources replaces a stale symlink with a correct one", async (t) => { + const { initResources } = await import("../resource-loader.ts"); + const tmp = mkdtempSync(join(tmpdir(), "gsd-symlink-stale-")); + const fakeAgentDir = join(tmp, "agent"); + + t.after(() => rmSync(tmp, { recursive: true, force: true })); + initResources(fakeAgentDir); + + const nodeModulesPath = join(fakeAgentDir, "node_modules"); + const correctTarget = readlinkSync(nodeModulesPath); + + // Remove and replace with a stale symlink pointing to a non-existent path + unlinkSync(nodeModulesPath); + symlinkSync("/tmp/nonexistent-gsd-node-modules-" + Date.now(), nodeModulesPath); + + const staleTarget = readlinkSync(nodeModulesPath); + assert.notEqual(staleTarget, correctTarget, "stale symlink should point elsewhere"); + + // Second call should fix the stale symlink + initResources(fakeAgentDir); + + const fixedTarget = readlinkSync(nodeModulesPath); + assert.equal(fixedTarget, correctTarget, "stale symlink should be replaced with correct target"); +}); + +test("initResources replaces symlink whose target was deleted", async (t) => { + const { initResources } = await import("../resource-loader.ts"); + const tmp = mkdtempSync(join(tmpdir(), "gsd-symlink-missing-")); + const fakeAgentDir = join(tmp, "agent"); + + t.after(() => rmSync(tmp, { recursive: true, force: true })); + initResources(fakeAgentDir); + + const nodeModulesPath = join(fakeAgentDir, "node_modules"); + const correctTarget = readlinkSync(nodeModulesPath); + + // Create a symlink that points to a path that doesn't exist + unlinkSync(nodeModulesPath); + const deadTarget = join(tmp, "old-install", "node_modules"); + symlinkSync(deadTarget, nodeModulesPath); + + // The symlink itself exists but its target doesn't + assert.equal(lstatSync(nodeModulesPath).isSymbolicLink(), true); + assert.equal(existsSync(deadTarget), false, "dead target should not exist"); + + initResources(fakeAgentDir); + + const fixedTarget = readlinkSync(nodeModulesPath); + assert.equal(fixedTarget, correctTarget, "broken symlink should be replaced with correct target"); +}); + +// --- Unit tests for pnpm-style merged node_modules (#3564) --- +// These simulate the filesystem layout without going through initResources, +// since packageRoot is fixed at module load time. + +test("pnpm layout: merged node_modules contains entries from both hoisted and internal", (t) => { + // Simulate pnpm global layout: + // hoisted/node_modules/ + // yaml/ ← external dep + // @sinclair/ ← external scoped dep + // gsd-pi/ ← package root + // node_modules/ + // @gsd/ ← workspace scope (NOT hoisted) + // @gsd-build/ ← workspace scope (NOT hoisted) + const tmp = mkdtempSync(join(tmpdir(), "gsd-pnpm-merge-")); + t.after(() => rmSync(tmp, { recursive: true, force: true })); + + const hoisted = join(tmp, "node_modules"); + const pkgRoot = join(hoisted, "gsd-pi"); + const internal = join(pkgRoot, "node_modules"); + const agentNodeModules = join(tmp, "agent", "node_modules"); + + // Create hoisted entries (external deps) + mkdirSync(join(hoisted, "yaml"), { recursive: true }); + mkdirSync(join(hoisted, "@sinclair", "typebox"), { recursive: true }); + mkdirSync(join(hoisted, "@anthropic-ai", "sdk"), { recursive: true }); + mkdirSync(pkgRoot, { recursive: true }); + + // Create internal entries (workspace packages) + mkdirSync(join(internal, "@gsd", "pi-ai"), { recursive: true }); + mkdirSync(join(internal, "@gsd", "pi-coding-agent"), { recursive: true }); + mkdirSync(join(internal, "@gsd-build", "core"), { recursive: true }); + + // Create merged directory manually (simulating what reconcileMergedNodeModules does) + mkdirSync(agentNodeModules, { recursive: true }); + + // Link hoisted entries (skip gsd-pi itself and dotfiles) + for (const entry of readdirSync(hoisted, { withFileTypes: true })) { + if (entry.name === "gsd-pi" || entry.name.startsWith(".")) continue; + symlinkSync(join(hoisted, entry.name), join(agentNodeModules, entry.name)); + } + + // Overlay internal entries (workspace scopes take precedence) + for (const entry of readdirSync(internal, { withFileTypes: true })) { + if (entry.name.startsWith(".")) continue; + const link = join(agentNodeModules, entry.name); + try { lstatSync(link); unlinkSync(link); } catch { /* didn't exist */ } + symlinkSync(join(internal, entry.name), link); + } + + // Verify: external deps resolve through hoisted symlinks + assert.ok(existsSync(join(agentNodeModules, "yaml")), "yaml should resolve"); + assert.ok(existsSync(join(agentNodeModules, "@sinclair")), "@sinclair should resolve"); + assert.ok(existsSync(join(agentNodeModules, "@anthropic-ai")), "@anthropic-ai should resolve"); + + // Verify: workspace packages resolve through internal symlinks + assert.ok(existsSync(join(agentNodeModules, "@gsd")), "@gsd should resolve"); + assert.ok(existsSync(join(agentNodeModules, "@gsd", "pi-ai")), "@gsd/pi-ai should resolve"); + assert.ok(existsSync(join(agentNodeModules, "@gsd-build")), "@gsd-build should resolve"); + + // Verify: gsd-pi itself is NOT symlinked (it's the package root, not a dep) + assert.ok(!existsSync(join(agentNodeModules, "gsd-pi")), "gsd-pi should not be in merged dir"); + + // Verify: @gsd points to internal, not hoisted (internal takes precedence) + const gsdTarget = readlinkSync(join(agentNodeModules, "@gsd")); + assert.equal(gsdTarget, join(internal, "@gsd"), "@gsd should point to internal node_modules"); +}); + +test("hasMissingWorkspaceScopes detects pnpm layout", (t) => { + const tmp = mkdtempSync(join(tmpdir(), "gsd-pnpm-detect-")); + t.after(() => rmSync(tmp, { recursive: true, force: true })); + + const hoisted = join(tmp, "hoisted"); + const internal = join(tmp, "internal"); + + // npm-style: @gsd exists in both hoisted and internal + mkdirSync(join(hoisted, "@gsd"), { recursive: true }); + mkdirSync(join(internal, "@gsd"), { recursive: true }); + + // Inline the detection logic for testing + const hasMissing = (h: string, i: string): boolean => { + if (!existsSync(i)) return false; + for (const entry of readdirSync(i, { withFileTypes: true })) { + if (entry.isDirectory() && entry.name.startsWith("@gsd") && + !existsSync(join(h, entry.name))) { + return true; + } + } + return false; + }; + + assert.equal(hasMissing(hoisted, internal), false, "npm-style: no missing scopes"); + + // pnpm-style: @gsd-build only in internal + mkdirSync(join(internal, "@gsd-build"), { recursive: true }); + assert.equal(hasMissing(hoisted, internal), true, "pnpm-style: @gsd-build missing from hoisted"); +}); + +test("merged node_modules marker prevents unnecessary rebuild", (t) => { + const tmp = mkdtempSync(join(tmpdir(), "gsd-pnpm-marker-")); + t.after(() => rmSync(tmp, { recursive: true, force: true })); + + const agentNodeModules = join(tmp, "agent", "node_modules"); + mkdirSync(agentNodeModules, { recursive: true }); + + // Write a marker file + const marker = join(agentNodeModules, ".gsd-merged"); + const fakePackageRoot = "/usr/lib/node_modules/gsd-pi"; + writeFileSync(marker, fakePackageRoot); + + // Verify the marker can be read back and matched + assert.equal(readFileSync(marker, "utf-8").trim(), fakePackageRoot); }); From a6286ac32ccf9d3ae3246e90a82ed0b5bc3a1fb5 Mon Sep 17 00:00:00 2001 From: Tibsfox Date: Sat, 11 Apr 2026 21:45:12 -0700 Subject: [PATCH 4/4] fix(cli): address review findings for pnpm merged node_modules - Use content fingerprint (packageRoot + sorted entry names from both dirs) in .gsd-merged marker so pnpm add/remove triggers rebuild - Restrict overlay loop to @gsd* scopes only, preventing accidental shadowing of hoisted deps with internal versions - Guard marker write behind linkedCount > 0 to avoid stamping success on a broken/empty merged directory - Log warnings when readdirSync fails on hoisted/internal roots Co-Authored-By: Claude Opus 4.6 (1M context) --- src/resource-loader.ts | 43 +++++++++++++++++++------- src/tests/node-modules-symlink.test.ts | 37 ++++++++++++++++------ 2 files changed, 60 insertions(+), 20 deletions(-) diff --git a/src/resource-loader.ts b/src/resource-loader.ts index a4c7e045c..234f26caa 100644 --- a/src/resource-loader.ts +++ b/src/resource-loader.ts @@ -357,7 +357,7 @@ function reconcileSymlink(link: string, target: string): void { /** * Create a real node_modules directory containing symlinks from both the - * hoisted root (external deps) and internal root (workspace packages). + * hoisted root (external deps) and internal root (@gsd/* workspace packages). * Used for pnpm global installs where @gsd/* isn't hoisted. */ function reconcileMergedNodeModules( @@ -365,10 +365,12 @@ function reconcileMergedNodeModules( hoisted: string, internal: string, ): void { - // Fast path: if already merged for this packageRoot, skip rebuild + // Fast path: if already merged for this packageRoot + same directory contents, skip. + // The fingerprint includes entry names from both roots so `pnpm add/remove` triggers rebuild. const marker = join(agentNodeModules, '.gsd-merged') + const fingerprint = mergedFingerprint(hoisted, internal) try { - if (existsSync(marker) && readFileSync(marker, 'utf-8').trim() === packageRoot) return + if (existsSync(marker) && readFileSync(marker, 'utf-8').trim() === fingerprint) return } catch { /* rebuild */ } // Remove any existing symlink or stale merged directory @@ -383,28 +385,47 @@ function reconcileMergedNodeModules( mkdirSync(agentNodeModules, { recursive: true }) + let linkedCount = 0 + // Symlink entries from the hoisted node_modules (external deps) try { for (const entry of readdirSync(hoisted, { withFileTypes: true })) { // Skip the gsd-pi package itself and dotfiles if (entry.name === basename(packageRoot)) continue if (entry.name.startsWith('.')) continue - try { symlinkSync(join(hoisted, entry.name), join(agentNodeModules, entry.name)) } catch { /* skip */ } + try { symlinkSync(join(hoisted, entry.name), join(agentNodeModules, entry.name)); linkedCount++ } catch { /* skip individual */ } } - } catch { /* non-fatal */ } + } catch (err) { + console.error(`[gsd] WARN: Failed to read hoisted node_modules at ${hoisted}: ${err instanceof Error ? err.message : err}`) + } - // Overlay workspace scopes from internal node_modules — these take precedence + // Overlay @gsd* workspace scopes from internal node_modules try { for (const entry of readdirSync(internal, { withFileTypes: true })) { - if (entry.name.startsWith('.')) continue + if (!entry.name.startsWith('@gsd')) continue const link = join(agentNodeModules, entry.name) try { lstatSync(link); unlinkSync(link) } catch { /* didn't exist */ } - try { symlinkSync(join(internal, entry.name), link) } catch { /* skip */ } + try { symlinkSync(join(internal, entry.name), link); linkedCount++ } catch { /* skip individual */ } } - } catch { /* non-fatal */ } + } catch (err) { + console.error(`[gsd] WARN: Failed to read internal node_modules at ${internal}: ${err instanceof Error ? err.message : err}`) + } - // Stamp marker so next startup can skip rebuild - try { writeFileSync(marker, packageRoot) } catch { /* non-fatal */ } + // Only stamp marker if we actually linked something — avoids caching a broken state + if (linkedCount > 0) { + try { writeFileSync(marker, fingerprint) } catch { /* non-fatal */ } + } +} + +/** Build a cache fingerprint from packageRoot + sorted entry names of both directories */ +function mergedFingerprint(hoisted: string, internal: string): string { + try { + const h = readdirSync(hoisted).sort().join(',') + const i = readdirSync(internal).sort().join(',') + return `${packageRoot}\n${h}\n${i}` + } catch { + return packageRoot // fallback: at least invalidate on version change + } } /** diff --git a/src/tests/node-modules-symlink.test.ts b/src/tests/node-modules-symlink.test.ts index 7f8beb8ca..a22f10910 100644 --- a/src/tests/node-modules-symlink.test.ts +++ b/src/tests/node-modules-symlink.test.ts @@ -147,9 +147,9 @@ test("pnpm layout: merged node_modules contains entries from both hoisted and in symlinkSync(join(hoisted, entry.name), join(agentNodeModules, entry.name)); } - // Overlay internal entries (workspace scopes take precedence) + // Overlay @gsd* workspace scopes from internal (these take precedence) for (const entry of readdirSync(internal, { withFileTypes: true })) { - if (entry.name.startsWith(".")) continue; + if (!entry.name.startsWith("@gsd")) continue; const link = join(agentNodeModules, entry.name); try { lstatSync(link); unlinkSync(link); } catch { /* didn't exist */ } symlinkSync(join(internal, entry.name), link); @@ -203,18 +203,37 @@ test("hasMissingWorkspaceScopes detects pnpm layout", (t) => { assert.equal(hasMissing(hoisted, internal), true, "pnpm-style: @gsd-build missing from hoisted"); }); -test("merged node_modules marker prevents unnecessary rebuild", (t) => { +test("merged node_modules marker uses fingerprint including directory entries", (t) => { const tmp = mkdtempSync(join(tmpdir(), "gsd-pnpm-marker-")); t.after(() => rmSync(tmp, { recursive: true, force: true })); + // Simulate two directories with known entries + const hoisted = join(tmp, "hoisted"); + const internal = join(tmp, "internal"); + mkdirSync(join(hoisted, "yaml"), { recursive: true }); + mkdirSync(join(hoisted, "@sinclair"), { recursive: true }); + mkdirSync(join(internal, "@gsd"), { recursive: true }); + + // Build fingerprint the same way the production code does + const h = readdirSync(hoisted).sort().join(","); + const i = readdirSync(internal).sort().join(","); + const fakePackageRoot = "/usr/lib/node_modules/gsd-pi"; + const fingerprint = `${fakePackageRoot}\n${h}\n${i}`; + const agentNodeModules = join(tmp, "agent", "node_modules"); mkdirSync(agentNodeModules, { recursive: true }); - - // Write a marker file const marker = join(agentNodeModules, ".gsd-merged"); - const fakePackageRoot = "/usr/lib/node_modules/gsd-pi"; - writeFileSync(marker, fakePackageRoot); + writeFileSync(marker, fingerprint); - // Verify the marker can be read back and matched - assert.equal(readFileSync(marker, "utf-8").trim(), fakePackageRoot); + // Verify fingerprint contains all three components + const stored = readFileSync(marker, "utf-8").trim(); + assert.ok(stored.includes(fakePackageRoot), "fingerprint includes packageRoot"); + assert.ok(stored.includes("@sinclair"), "fingerprint includes hoisted entries"); + assert.ok(stored.includes("@gsd"), "fingerprint includes internal entries"); + + // Verify fingerprint changes when a new package is added + mkdirSync(join(hoisted, "new-package"), { recursive: true }); + const h2 = readdirSync(hoisted).sort().join(","); + const fingerprint2 = `${fakePackageRoot}\n${h2}\n${i}`; + assert.notEqual(fingerprint, fingerprint2, "fingerprint should change when deps change"); });