golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(#2624): write the .gsd-source marker before staging reads it (#2811)

Browse Source 
* test(#2624): failing-first regression for stale .gsd-source marker read-before-write

Adds failing-first regression proving a Claude-global upgrade currently lets
staging read a stale prior-version .gsd-source marker before install() rewrites
it. Pre-seeds a stale marker pointing at a still-existing fake source, spies on
findInstallSourceRoot to capture which source staging resolves, and asserts
staging never resolves the stale path. Also covers fresh-install and ghost-marker
negative space.

* fix(#2624): write the .gsd-source marker before staging reads it

A Claude-global upgrade silently installed skill content from the PREVIOUS
version. findInstallSourceRoot prefers <configDir>/.gsd-source, and install()
used to rewrite that marker AFTER staging had already read it — so on an
upgrade the marker still pointed at the prior version's source (an npx cache
dir that still exists on disk) and every converted skill was generated from
the OLD commands/gsd, with generateManifest then recording the stale content's
hash as correct.

Extract the marker write into _writeGsdSourceMarker(runtime, targetDir, src,
isGlobal) and call it BEFORE the staging pass (before the _isSkillsRuntime
branch), preserving the original sourceMarkerFile && isGlobal guard, the
half-published-package existsSync guard, and the non-fatal write-failure warn.
This closes the read-before-write hole for every findInstallSourceRoot
consumer (skills, commands, /gsd-surface, capability-state) in one move.

Long-standing (marker write added in ee6f3b70c / #1477, already in v1.7.0);
triggers on any Claude-global upgrade where skill content changed and the
prior source path still exists (the common npx-persistent-cache case).

* test(#2624): use t.mock.method for process.exit/console per CONTRIBUTING rules

Address code-review finding: replace manual process.exit/console monkeypatch
and try/finally in the #2624 test helpers with t.mock.method (auto-restored),
matching CONTRIBUTING.md test conventions and the t.mock.method idiom used
elsewhere in the suite.

* docs(changeset): #2624 install-source-root-stale-marker

* docs(changeset): backfill #2624 PR number to 2811
This commit is contained in:
Tom Boucher
2026-07-28 23:09:10 -04:00
committed by GitHub
parent 7f13ee5373
commit 46bae2f9ff
3 changed files with 189 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/2624-install-source-root-stale-marker.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 2811
---
**Upgrading a Claude-global GSD install now uses the new version's skill content instead of the previous version's** — the installer read a `.gsd-source` marker that still pointed at the prior install's source location before rewriting it, so on an upgrade every converted skill was generated from the old version's command definitions (while the file manifest faithfully recorded the stale content's hash as correct). The marker is now written before anything reads it. (#2624)

73
bin/install.js
View File

@@ -10406,6 +10406,45 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
return Array.isArray(scopeLayout) && scopeLayout.length > 0;
})();
// #2624: write the .gsd-source marker. Extracted from its former late position so it can be
// called BEFORE staging reads the marker (see the call site below). Scoped to the Claude-global
// layout (issue #1477) — the only install path that ships the skills layout without a
// commands/gsd source tree, so findInstallSourceRoot's walk-up has nothing to find and
// /gsd-surface (list/status) throws without it. Points at the package's own commands/gsd
// source. Guarded on source presence so a half-published package never writes a dangling
// marker. Write failure is non-fatal (install proceeds; warn so /gsd-surface breakage is
// diagnosable) — the same contract the late write had.
function _writeGsdSourceMarker(runtime, targetDir, src, isGlobal) {
if (_hostBehaviors(runtime).sourceMarkerFile && isGlobal) {
const gsdSourceCommands = path.join(src, 'commands', 'gsd');
if (fs.existsSync(gsdSourceCommands)) {
try {
// ADR-1239 Phase B write-confinement: the descriptor-sourced marker filename
// must resolve under targetDir (parity with the other descriptor-driven writes).
const _markerPath = assertDestWithinConfigHome(targetDir, _hostBehaviors(runtime).sourceMarkerFile);
fs.writeFileSync(_markerPath, gsdSourceCommands + '\n', 'utf8');
} catch (err) {
// Non-fatal: install proceeds. But on the Claude-global layout walk-up
// also fails (no commands/gsd source tree), so a silent write failure
// still leaves /gsd-surface broken at runtime — warn so it's diagnosable.
console.warn(` ${yellow}!${reset} Could not write .gsd-source marker (${err.message}); /gsd-surface list/status may fail`);
}
}
}
}
// #2624: write the .gsd-source marker BEFORE any staging reads it. The marker write
// formerly lived AFTER staging; on an upgrade the marker still held the PREVIOUS
// install's source path (e.g. an npx per-version cache dir that still exists on disk),
// so findInstallSourceRoot(configDir) — called inside installRuntimeArtifacts below —
// returned the stale path and every converted skill was generated from the OLD version's
// commands/gsd, silently installing prior-version content with a self-consistent manifest
// hash. Writing first closes the read-before-write hole for every findInstallSourceRoot
// consumer (skills, commands, /gsd-surface, capability-state). Placed here (before the
// _isSkillsRuntime branch) so it runs for every Claude-global install, matching the
// original write's sourceMarkerFile && isGlobal guard exactly.
_writeGsdSourceMarker(runtime, targetDir, src, isGlobal);
if (_isSkillsRuntime) {
// Layout-driven install for skills-based runtimes (full and minimal modes)
const scope = isGlobal ? 'global' : 'local';
@@ -10694,35 +10733,11 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
failures.push('gsd-core');
}
// Write the .gsd-source marker so runtime source resolution succeeds at
// runtime (#1477). The Claude-global skills layout ships gsd-core/{bin,
// contexts,references,templates,workflows} but NOT the commands/gsd source
// tree, and _runLegacyUninstallCleanup actively removes any commands/gsd/
// for that scope — so findInstallSourceRoot's walk-up has nothing to find
// and /gsd-surface (list/status) throws. This is the writer half of the
// marker that runtime-artifact-layout.cjs's finders already read (the reader
// landed in #1476). It points at the package's own commands/gsd source.
// Scoped to the Claude-global layout (issue #1477) — the only install path
// that ships the skills layout without a commands/gsd source tree; every
// other runtime/scope deploys commands/gsd, so its walk-up already resolves
// and needs no marker. Guarded on source presence so a half-published
// package never writes a dangling marker.
if (_hostBehaviors(runtime).sourceMarkerFile && isGlobal) {
const gsdSourceCommands = path.join(src, 'commands', 'gsd');
if (fs.existsSync(gsdSourceCommands)) {
try {
// ADR-1239 Phase B write-confinement: the descriptor-sourced marker filename
// must resolve under targetDir (parity with the other descriptor-driven writes).
const _markerPath = assertDestWithinConfigHome(targetDir, _hostBehaviors(runtime).sourceMarkerFile);
fs.writeFileSync(_markerPath, gsdSourceCommands + '\n', 'utf8');
} catch (err) {
// Non-fatal: install proceeds. But on the Claude-global layout walk-up
// also fails (no commands/gsd source tree), so a silent write failure
// still leaves /gsd-surface broken at runtime — warn so it's diagnosable.
console.warn(` ${yellow}!${reset} Could not write .gsd-source marker (${err.message}); /gsd-surface list/status may fail`);
}
}
}
// #2624: the .gsd-source marker is now written by _writeGsdSourceMarker()
// BEFORE staging reads it (see the early call above the _isSkillsRuntime
// block). The former write lived here — AFTER staging — which on an upgrade
// let staging read a stale prior-version marker and silently install
// old-version skill content. Moved up; this site intentionally left empty.
// #1629 critical fix: Windsurf workflow wrappers (convertClaudeCommandToWindsurfWorkflow)
// delegate to command bodies at <targetDir>/gsd-core/commands/gsd/${stem}.md via a

140
tests/runtime-artifact-layout.test.cjs
View File

@@ -936,3 +936,143 @@ describe('#1477 .gsd-source marker provisioning', () => {
});
});
});
// ─── #2624: stale .gsd-source marker read before rewrite on upgrade ──────────
//
// Regression for #2624: a Claude-global upgrade silently installed skill content
// from the PREVIOUS version. findInstallSourceRoot prefers <configDir>/.gsd-source,
// and install() used to REWRITE that marker AFTER staging had already read it — so
// on an upgrade the marker still pointed at the prior version's source (an npx
// cache dir that still exists on disk) and every converted skill was generated from
// the OLD commands/gsd. Fix: write the marker BEFORE staging reads it.
//
// These exercise the real install(true, 'claude') with HOME redirected to a tmp dir,
// pre-seeding a STALE marker that points at a different (still-existing) source —
// the exact upgrade condition. No live npx / network.
describe('#2624 .gsd-source marker is rewritten before staging reads it', () => {
let tmpRoot;
let savedHome;
let savedUserProfile;
let savedExplicitConfigDir;
let savedTestMode;
// Run install() with process.exit and console output mocked via t.mock (auto-restored),
// per CONTRIBUTING.md test rules (no manual monkeypatch / try-finally in test bodies).
// process.exit during install is a hard failure — surface it, don't let it kill the runner.
function runInstall(t, isGlobal, runtime) {
t.mock.method(process, 'exit', (code) => {
throw new Error(`process.exit(${code}) during install — should not happen`);
});
t.mock.method(console, 'log', () => {});
t.mock.method(console, 'warn', () => {});
t.mock.method(console, 'error', () => {});
return install(isGlobal, runtime);
}
beforeEach(() => {
tmpRoot = createTempDir('gsd-2624-');
savedHome = process.env.HOME;
savedUserProfile = process.env.USERPROFILE;
process.env.HOME = tmpRoot;
process.env.USERPROFILE = tmpRoot;
savedExplicitConfigDir = process.env.GSD_EXPLICIT_CONFIG_DIR;
delete process.env.GSD_EXPLICIT_CONFIG_DIR;
savedTestMode = process.env.GSD_TEST_MODE;
process.env.GSD_TEST_MODE = '1';
});
afterEach(() => {
if (savedHome === undefined) delete process.env.HOME;
else process.env.HOME = savedHome;
if (savedUserProfile === undefined) delete process.env.USERPROFILE;
else process.env.USERPROFILE = savedUserProfile;
if (savedExplicitConfigDir === undefined) delete process.env.GSD_EXPLICIT_CONFIG_DIR;
else process.env.GSD_EXPLICIT_CONFIG_DIR = savedExplicitConfigDir;
if (savedTestMode === undefined) delete process.env.GSD_TEST_MODE;
else process.env.GSD_TEST_MODE = savedTestMode;
cleanup(tmpRoot);
});
// The current package's commands/gsd — what the marker MUST point at after install.
const CURRENT_SOURCE = path.join(REPO_ROOT, 'commands', 'gsd');
test('claude-global install overwrites a stale .gsd-source marker before staging reads it', (t) => {
const claudeDir = path.join(tmpRoot, '.claude');
fs.mkdirSync(claudeDir, { recursive: true });
// Simulate the PREVIOUS install's marker: a different source dir that STILL EXISTS
// on disk (mirroring a coexisting npx per-version cache dir). findInstallSourceRoot
// returns this immediately if it is read before the marker is rewritten.
const staleSource = path.join(tmpRoot, 'old-npx-cache', 'commands', 'gsd');
fs.mkdirSync(staleSource, { recursive: true });
fs.writeFileSync(path.join(claudeDir, '.gsd-source'), staleSource + '\n', 'utf8');
// Spy on findInstallSourceRoot to capture WHICH source staging actually resolves.
// The marker ends up correct either way (the late write corrected it on the old code),
// so the marker content alone cannot prove the ordering — the resolved-source captures
// can. Before the fix, staging resolves the stale path; after, the current path.
// install-engine.cjs calls runtimeArtifactLayout.findInstallSourceRoot via the module
// object (not a captured ref), so mocking the property on the shared module instance
// intercepts the staging call. Same pattern as tests/phase.test.cjs (capture original,
// delegate).
const runtimeArtifactLayout = require('../gsd-core/bin/lib/runtime-artifact-layout.cjs');
const realFindInstallSourceRoot = runtimeArtifactLayout.findInstallSourceRoot;
const resolvedSources = [];
t.mock.method(runtimeArtifactLayout, 'findInstallSourceRoot', function (configDir) {
const result = realFindInstallSourceRoot.call(this, configDir);
resolvedSources.push(path.resolve(result));
return result;
});
runInstall(t, true /* isGlobal */, 'claude');
const markerPath = path.join(claudeDir, '.gsd-source');
assert.ok(fs.existsSync(markerPath), 'marker must exist after install');
const finalMarker = path.resolve(fs.readFileSync(markerPath, 'utf8').trim());
assert.equal(finalMarker, path.resolve(CURRENT_SOURCE),
`final marker must point at the current package source, not ${finalMarker}`);
// The decisive assertion: staging must NEVER have resolved the stale source. Before the
// fix, at least one staging resolution returned the stale path (read before rewrite).
const resolvedStale = resolvedSources.filter((p) => p === path.resolve(staleSource));
assert.equal(resolvedStale.length, 0,
`staging must not resolve the stale source during install, but did ${resolvedStale.length} time(s). ` +
`Resolved: ${JSON.stringify(resolvedSources)}`);
});
test('fresh claude-global install (no prior marker) still writes the correct marker', (t) => {
const claudeDir = path.join(tmpRoot, '.claude');
fs.mkdirSync(claudeDir, { recursive: true });
// No pre-existing marker — fresh install (negative space, must stay correct).
runInstall(t, true /* isGlobal */, 'claude');
const markerPath = path.join(claudeDir, '.gsd-source');
assert.ok(fs.existsSync(markerPath), 'fresh claude-global install must write the marker');
const resolved = fs.readFileSync(markerPath, 'utf8').trim();
assert.equal(
path.resolve(resolved),
path.resolve(CURRENT_SOURCE),
'fresh install marker must point at the current package source',
);
});
test('claude-global install rewrites a marker pointing at a deleted (ghost) source', (t) => {
const claudeDir = path.join(tmpRoot, '.claude');
fs.mkdirSync(claudeDir, { recursive: true });
// A ghost path that does NOT exist on disk — findInstallSourceRoot ignores it and
// falls through to walk-up, then install() rewrites the marker to the current source.
const ghost = path.join(tmpRoot, 'gone', 'commands', 'gsd');
fs.writeFileSync(path.join(claudeDir, '.gsd-source'), ghost + '\n', 'utf8');
runInstall(t, true /* isGlobal */, 'claude');
const markerPath = path.join(claudeDir, '.gsd-source');
assert.ok(fs.existsSync(markerPath), 'marker must exist after install');
const resolved = fs.readFileSync(markerPath, 'utf8').trim();
assert.equal(
path.resolve(resolved),
path.resolve(CURRENT_SOURCE),
'ghost marker must be rewritten to the current package source',
);
});
});