From a6286ac32ccf9d3ae3246e90a82ed0b5bc3a1fb5 Mon Sep 17 00:00:00 2001 From: Tibsfox Date: Sat, 11 Apr 2026 21:45:12 -0700 Subject: [PATCH] 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"); });