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) <noreply@anthropic.com>
This commit is contained in:
Tibsfox 2026-04-11 21:45:12 -07:00
parent 42d2e25e0b
commit a6286ac32c
2 changed files with 60 additions and 20 deletions

View file

@ -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
}
}
/**

View file

@ -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");
});