* enhance(#4377): opt-in project-relative includes for local installs A local install wrote the includes that point at GSD's own files as absolute paths — whatever the installer resolved at install time. For one checkout that is invisible. Across git worktrees it is not: each worktree gets its own .claude/ copy, but all of them point back at the checkout that ran the installer, so a worktree runs its own gsd-tools.cjs while reading workflow prose from a different checkout. Update that one checkout and every other worktree is running new instructions against an old engine, with nothing to stage the update with. --relative-includes (or GSD_RELATIVE_INCLUDES=1) makes a local install emit `@.claude/gsd-core/...`. Opt-in, and staying opt-in: absolute works for a single checkout, which is most people, and flipping the default would change every existing local install to solve a problem those users do not have. The prefix is the runtime's own localConfigDir descriptor value, never a literal — the same value resolveScope joins onto the cwd to produce the install target, and the same one the rewrite engine already uses for its ./.claude/ -> ./<dir>/ substitutions. Copilot and Antigravity have shipped this shape for local installs since they were added, with hardcoded .github/ and .agents/; this is that behavior, derived rather than written down. Six seams compute a path prefix and all six had to be threaded, which is why the opt-in travels through the environment the way --portable-hooks already does: one variable they all read cannot fall out of sync the way six signatures can. The launcher shim deliberately keeps its ABSOLUTE fallbacks. It probes gsd-tools through ${CLAUDE_CONFIG_DIR:-$HOME/.claude} and one such default per runtime; those are shell word expansions, not includes, and a relative value there resolves against the shell's cwd rather than the project. Trading an include that points at the wrong checkout for a path that points at nothing is not a fix. All three rewrite paths mask ${VAR:-default} spans before substituting and restore them after, and the mask only runs when the prefix is relative, so an absolute install is byte-for-byte unchanged. Every unexpressible case falls back to absolute: no opt-in, a global install, a missing dir name, the configHome.kind === 'none' sentinel, an absolute descriptor value, or one climbing out of the project with '..'. * chore(#4377): add changeset for project-relative local includes * fix(#4377): compare against POSIX-normalized roots in the install e2e arms The emitted prefix is POSIX-normalized by design — it is substituted into markdown @-references, which use forward slashes universally, so a backslash would leak into shipped content (#1615). The e2e arms compared against the raw temp root, which on Windows is `D:\a\...` and appears in no emitted file. That reddened the control arm on the windows shard, and it was worse than a red: the negative arm ("nothing references the checkout") was passing VACUOUSLY there, because a string that cannot occur is trivially absent. Both now go through the same normalization, so the Windows lane asserts what the Linux lane does. * fix(#4377): tolerate a resolved temp root, and make the e2e diff self-diagnosing Two changes, one confirmed and one to stop guessing. Confirmed: the emitted content carries the RESOLVED root, not the spelling mkdtemp handed back. Reproduced on Linux with a symlinked install root — 236 emitted files carry the realpath, zero carry the link path. macOS has this structurally, since /var is a symlink to /private/var. Comparisons now go through both spellings, or the negative arms pass vacuously: "nothing references the checkout" is trivially true when the string being searched for cannot occur. Not confirmed: the macOS shard reported ~every workflow file differing in the "differ ONLY" arm while the five arms around it passed, and the assertion printed a list of filenames — which says a difference exists somewhere across 236 files and leaves the reader to guess which bytes. I cannot reproduce that platform locally, and guessing turns one CI round-trip into four. The assertion now reports the first divergence as text: the file, the byte offset, and a bounded window of both sides. * fix(#4377): strip the longest root spelling first in the install e2e diff The macOS failure was my test corrupting its own comparison, not a product defect. /var/folders/…/X is a SUBSTRING of /private/var/folders/…/X, so stripping the unresolved spelling first matched inside the resolved one and left the /private prefix glued to what followed: @/private/var/…/X/.claude/gsd-core/… -> @/private.claude/gsd-core/… a string present in neither install, which is why all 236 files "differed". Sorting the spellings longest-first consumes the whole occurrence, and the short form then has nothing left to match. Proven in isolation on the exact macOS shapes: short-first yields @/private.claude/…, longest-first yields @.claude/…. The self-diagnosing assertion added in the previous commit is what found this — it named the file, the byte offset, and printed both sides, so the corrupted string was visible rather than inferred from a list of 236 filenames. Keeping it. * fix(#4377): address review findings * test(#4377): scan nested shell defaults without regex backtracking * fix(#4377): close relative include review gaps * fix(#4377): preserve root-target runtime includes * fix(#4377): guard project-root relative includes * test(#4377): normalize Cline fallback roots * fix(#4377): persist relative include style --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Michel Moreira
@@ -8204,3 +8204,223 @@ describe('#607 cleanupLegacyGsdCc: exported helper unit tests', () => {
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// #4377 — --relative-includes: a local install must not bake the checkout path
|
||||
// ---------------------------------------------------------------------------
|
||||
//
|
||||
// The unit arms in tests/install-runtime-artifacts.test.cjs pin
|
||||
// _computePathPrefix itself. This is the end-to-end proof: the prefix has to
|
||||
// travel from the CLI flag, through the install engine, through the rewrite
|
||||
// pass, and land in the bytes on disk. A pure-function test alone would pass
|
||||
// happily while any one of those six seams kept computing the absolute form.
|
||||
describe('#4377: --relative-includes emits project-relative @ includes for a local install', () => {
|
||||
// `before`/`after` are not in this file's tail scope (the earlier folds each
|
||||
// bring their own); pull them from node:test directly rather than relying on
|
||||
// whatever binding happens to be visible here.
|
||||
const { before: beforeAll, after: afterAll } = require('node:test');
|
||||
const { installSpawnEnv } = require('./helpers.cjs');
|
||||
const { throwIfFailed } = require('./helpers/git-fixture.cjs');
|
||||
const { INSTALL_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
|
||||
const installPath = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
|
||||
// Two independent reasons the raw temp root does not appear in emitted
|
||||
// content, both of which would make the negative arms pass VACUOUSLY —
|
||||
// "nothing references the checkout" is trivially true when the string being
|
||||
// searched for cannot occur:
|
||||
//
|
||||
// 1. The emitted prefix is POSIX-normalized by design, because it is
|
||||
// substituted into markdown @-references, which use forward slashes
|
||||
// universally; a backslash there would leak into shipped content
|
||||
// (#1615). On Windows the roots arrive as `D:\a\...`.
|
||||
// 2. On macOS `os.tmpdir()` yields `/var/folders/...`, but `/var` is a
|
||||
// symlink to `/private/var`, and the installer resolves it — so the
|
||||
// emitted content carries `/private/var/folders/...` while
|
||||
// `fs.mkdtempSync` handed us the unresolved spelling.
|
||||
//
|
||||
// Every comparison therefore goes through both spellings.
|
||||
const toPosix = (p2) => p2.replace(/\\/g, '/');
|
||||
//
|
||||
// LONGEST FIRST, and that ordering is load-bearing. `/var/folders/…/X` is a
|
||||
// SUBSTRING of `/private/var/folders/…/X`, so stripping the short spelling
|
||||
// first matches inside the long one and leaves the `/private` prefix glued
|
||||
// to what followed it: `@/private/var/…/X/.claude/gsd-core/…` normalizes to
|
||||
// `@/private.claude/gsd-core/…`, a string that appears in neither install.
|
||||
// Removing the long form first consumes the whole occurrence, and the short
|
||||
// form then has nothing left to match.
|
||||
const rootSpellings = (root) => {
|
||||
const forms = new Set([toPosix(root)]);
|
||||
try { forms.add(toPosix(fs.realpathSync(root))); } catch { /* root already gone */ }
|
||||
return [...forms].sort((a, b) => b.length - a.length);
|
||||
};
|
||||
const mentionsRoot = (content, root) => rootSpellings(root).some((r) => content.includes(r));
|
||||
|
||||
// Self-contained runner: the file's other runInstall helpers live inside
|
||||
// folded blocks and are not in scope here. #3156's sandboxed HOME still
|
||||
// applies — the installer writes <home>/.gsd/defaults.json via os.homedir()
|
||||
// directly, which no env scrub reaches.
|
||||
const install = (cwd, args) => {
|
||||
const env = installSpawnEnv();
|
||||
delete env.GSD_TEST_MODE;
|
||||
// The relative-includes opt-in is dual-sourced (flag OR env), so an
|
||||
// ambient GSD_RELATIVE_INCLUDES would silently opt the CONTROL arm in and
|
||||
// make this suite prove nothing. Scrub it and let the flag speak.
|
||||
delete env.GSD_RELATIVE_INCLUDES;
|
||||
const r = runNode([installPath, ...args], {
|
||||
cwd, env, timeoutMs: INSTALL_TIMEOUT_MS,
|
||||
});
|
||||
throwIfFailed(r, `node ${installPath} ${args.join(' ')}`);
|
||||
};
|
||||
|
||||
let absDir;
|
||||
let relDir;
|
||||
|
||||
beforeAll(() => {
|
||||
absDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4377-abs-'));
|
||||
relDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4377-rel-'));
|
||||
install(absDir, ['--claude', '--local', '--no-sdk']);
|
||||
install(relDir, ['--claude', '--local', '--no-sdk', '--relative-includes']);
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
cleanup(absDir);
|
||||
cleanup(relDir);
|
||||
});
|
||||
|
||||
// Walk the WHOLE installed tree, not just commands/. The issue measured 145
|
||||
// affected files across commands, workflows and references on a real repo —
|
||||
// a commands-only assertion would have called this fixed while 124 workflow
|
||||
// and reference files still carried the baked path.
|
||||
const allBodies = (root, base = path.join(root, '.claude')) => {
|
||||
const out = [];
|
||||
const walk = (dir) => {
|
||||
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
|
||||
const full = path.join(dir, entry.name);
|
||||
if (entry.isDirectory()) { walk(full); continue; }
|
||||
if (!entry.isFile()) continue;
|
||||
out.push({ rel: path.relative(base, full), content: fs.readFileSync(full, 'utf-8') });
|
||||
}
|
||||
};
|
||||
if (fs.existsSync(base)) walk(base);
|
||||
return out;
|
||||
};
|
||||
|
||||
test('the default install still bakes the absolute checkout path (unchanged behavior)', () => {
|
||||
// The control. Without it, a change that broke the absolute form entirely
|
||||
// would satisfy every assertion below and look like a fix.
|
||||
const carriers = allBodies(absDir).filter((f) => mentionsRoot(f.content, absDir));
|
||||
assert.ok(
|
||||
carriers.length > 0,
|
||||
'#4377 is opt-in: the default local install must keep emitting absolute includes',
|
||||
);
|
||||
});
|
||||
|
||||
test('with the flag, NOTHING in the installed tree references the checkout it came from', () => {
|
||||
const offenders = allBodies(relDir).filter((f) => mentionsRoot(f.content, relDir)).map((f) => f.rel);
|
||||
assert.deepEqual(offenders, [], 'no installed file may reference the checkout it was installed from');
|
||||
});
|
||||
|
||||
test('with the flag, the includes are project-relative and still resolve to gsd-core', () => {
|
||||
// Not merely "absent" — an implementation that stripped the prefix would
|
||||
// pass the arm above while shipping includes that point nowhere.
|
||||
const withRelative = allBodies(relDir).filter((f) => f.content.includes('@.claude/gsd-core/'));
|
||||
assert.ok(withRelative.length > 0, 'expected @.claude/gsd-core/ includes in the relative install');
|
||||
});
|
||||
|
||||
test('the local install manifest persists the relative style for a later surface apply', () => {
|
||||
const manifest = JSON.parse(fs.readFileSync(path.join(relDir, '.claude', 'gsd-file-manifest.json'), 'utf8'));
|
||||
assert.equal(manifest.relativeIncludePrefix, '.claude/');
|
||||
});
|
||||
|
||||
test('a project-root runtime falls back to absolute includes instead of inventing its descriptor directory', (t) => {
|
||||
// Cline declares localTargetIsProjectRoot: local agents live in `agents/`,
|
||||
// not `.cline/agents/`. A relative `.cline/` prefix would therefore point
|
||||
// at a directory the install never creates. This crosses the real install
|
||||
// plan and agent-rewrite seam; testing _localIncludeDirName alone would not
|
||||
// prove every production consumer uses the guard.
|
||||
const clineDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4377-cline-'));
|
||||
t.after(() => cleanup(clineDir));
|
||||
install(clineDir, ['--cline', '--local', '--no-sdk', '--relative-includes']);
|
||||
|
||||
const agents = allBodies(clineDir, path.join(clineDir, 'agents'));
|
||||
assert.ok(agents.length > 0, 'the local Cline install must emit agents at the project root');
|
||||
const descriptorRelative = agents.filter((f) => f.content.includes('@.cline/gsd-core/')).map((f) => f.rel);
|
||||
assert.deepEqual(descriptorRelative, [], 'Cline has no .cline/ local target, so its includes must not use one');
|
||||
const absoluteFallback = agents.filter((f) => rootSpellings(clineDir)
|
||||
.some((root) => f.content.includes(`@${root}/gsd-core/`)));
|
||||
assert.ok(absoluteFallback.length > 0, 'an unrepresentable project-relative target must retain the safe absolute prefix');
|
||||
});
|
||||
|
||||
test('the launcher shim keeps ABSOLUTE shell defaults even with the flag', () => {
|
||||
// The one place a relative prefix would make things worse. The runtime
|
||||
// launcher probes gsd-tools through `${CLAUDE_CONFIG_DIR:-$HOME/.claude}`;
|
||||
// that is a shell word expansion, not an @ include, and a bare `.claude`
|
||||
// there resolves against the shell's cwd rather than the project. Trading
|
||||
// an include that points at the wrong checkout for a path that points at
|
||||
// nothing would not be a fix.
|
||||
const carriers = allBodies(relDir).filter((f) => f.content.includes('CLAUDE_CONFIG_DIR:-'));
|
||||
assert.ok(carriers.length > 0, 'expected the launcher snippet in the installed tree');
|
||||
const offenders = carriers
|
||||
.filter((f) => !/CLAUDE_CONFIG_DIR:-\$HOME\/\.claude\}/.test(f.content))
|
||||
.map((f) => f.rel);
|
||||
assert.deepEqual(offenders, [], 'the launcher fallback must stay $HOME-absolute');
|
||||
});
|
||||
|
||||
test('no masking sentinel survives into the installed tree', () => {
|
||||
// The shell-default guard masks with a literal token before rewriting and
|
||||
// restores after. A restore that missed would ship that token as content.
|
||||
// gsd-core/bin/lib/ carries the implementation itself, so exclude it.
|
||||
const leaked = allBodies(relDir)
|
||||
.filter((f) => !f.rel.startsWith(path.join('gsd-core', 'bin', 'lib')))
|
||||
.filter((f) => f.content.includes('@@GSD4377:'))
|
||||
.map((f) => f.rel);
|
||||
assert.deepEqual(leaked, [], 'the shell-default mask must be fully restored');
|
||||
});
|
||||
|
||||
test('the two installs differ ONLY where the flag is meant to change things', () => {
|
||||
// The strongest arm. Normalize the absolute install into what the relative
|
||||
// one should be — undo the two deliberate differences — and the trees must
|
||||
// then be byte-identical. Anything ELSE the flag touched surfaces here
|
||||
// instead of going unnoticed.
|
||||
const abs = allBodies(absDir);
|
||||
const rel = new Map(allBodies(relDir).map((f) => [f.rel, f.content]));
|
||||
assert.ok(abs.length > 0, 'the control install must produce files');
|
||||
const mismatched = [];
|
||||
for (const { rel: name, content } of abs) {
|
||||
if (!rel.has(name)) { mismatched.push(`${name} (missing from relative install)`); continue; }
|
||||
// Both manifests record per-file absolute paths, which legitimately
|
||||
// differ between two different install roots.
|
||||
if (name === 'gsd-file-manifest.json' || name === 'gsd-install-state.json') continue;
|
||||
let normalized = content;
|
||||
for (const absRoot of rootSpellings(absDir)) {
|
||||
normalized = normalized
|
||||
// (1) shell defaults: the absolute install rewrote them to its own
|
||||
// root; the relative install left them at $HOME.
|
||||
.split(`:-${absRoot}/.claude}`).join(':-$HOME/.claude}')
|
||||
// (2) the include prefix itself, both the slash form and the bare
|
||||
// trailing form the word-boundary passes emit.
|
||||
.split(`${absRoot}/.claude/`).join('.claude/')
|
||||
.split(`${absRoot}/.claude`).join('.claude');
|
||||
}
|
||||
if (normalized !== rel.get(name)) mismatched.push({ name, normalized, actual: rel.get(name) });
|
||||
}
|
||||
// Report the FIRST divergence as text, not just a list of filenames. A
|
||||
// bare file list says a difference exists somewhere in 236 files and
|
||||
// leaves the reader to guess which bytes — and on a platform I cannot
|
||||
// reproduce locally, guessing is what turns one CI round-trip into four.
|
||||
const detail = mismatched.length === 0 ? '' : (() => {
|
||||
const { name, normalized, actual } = mismatched[0];
|
||||
let at = 0;
|
||||
while (at < normalized.length && at < actual.length && normalized[at] === actual[at]) at += 1;
|
||||
const from = Math.max(0, at - 60);
|
||||
return `\nfirst divergence in ${name} at offset ${at}:\n`
|
||||
+ ` absolute(normalized): ${JSON.stringify(normalized.slice(from, at + 120))}\n`
|
||||
+ ` relative(actual): ${JSON.stringify(actual.slice(from, at + 120))}\n`
|
||||
+ `(${mismatched.length} file(s) differ: ${mismatched.slice(0, 8).map((m) => m.name).join(', ')}${mismatched.length > 8 ? ', …' : ''})`;
|
||||
})();
|
||||
assert.deepEqual(
|
||||
mismatched.map((m) => m.name), [],
|
||||
`the flag must change the include prefix, and nothing beyond it${detail}`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user