* test(#2870): failing-first suite for the Install Scope Module 19 tests over the 50-test-matrix rows 1-19. RED by construction: the module under test does not exist yet, so the suite fails at require with MODULE_NOT_FOUND until src/install-scope.cts lands. Every row asserts a returned value with injected env/home/existsSync -- no filesystem, per the issue's acceptance criterion that tests assert the resolved value directly. Row 7 asserts the RELATION rank(global) > rank(local) rather than a literal, so Phase 2 (#2871) can re-base the numbers without a fixture edit. Row 4 iterates the real runtime registry rather than a hardcoded list, excluding vscode, which declares configHome.kind none and is never CLI-installed. * feat(#2870): add the Install Scope Module Scope becomes one resolved value instead of a bare string re-derived at every layer. resolveScope({id, runtime, ...}) returns {id, configHome, settingsFile, consentRequired, hostPrecedenceRank}. It COMPOSES resolveConfigHomeFromDescriptor rather than extending it. That function has 60 dependents across 13 files and 2 process flows -- a CRITICAL blast radius -- so adding a scope parameter to it, which the issue's framing invites, would ripple through all of them. Composing costs nothing and leaves every existing caller byte-identical. The module owns the InstallScope type name, which previously lived privately in runtime-artifact-install-plan.cts; that module now imports it. A fifth spelling of the same concept would have defeated the phase. settingsFile is null for the 18 runtimes that declare no settingsFileByScope -- absence is a value, not an error, and inventing a Claude-shaped default would leak that host's shape onto every other one. hostPrecedenceRank ships unread: Phase 2 (#2871) is its first consumer. It is carried as data only, per this issue's out-of-scope note that precedence semantics belong to that phase. Vocabulary: the install axis standardizes on local. ConsentRecord.scope keeps project deliberately -- that literal is serialized into consent records in the user's home, and renaming it would silently deactivate every project-scoped capability on the machine. CONTEXT.md records the boundary mapping instead. Every environmental input is injectable (env, home, existsSync, cwd), so the resolved value is assertable with no filesystem at all. Registration ripple: .gitignore, eslint.config.mjs, CONTEXT.md glossary, docs/INVENTORY.md, and the inventory manifest (regenerated after build:lib, never before). Verified via the remote runner. * refactor(#2870): route scope re-derivations through the module bin/install.js resolves scope once per function instead of inline at each of its 12 sites, and the settingsFileByScope consumer reads it through resolveScope(). Seven downstream boolean re-derivations now call the module's isGlobalScope() instead of comparing the literal independently: runtime-artifact-install-plan, both runtime-artifact-layout kind builders, dispatchKindEntry, surface, and two install-engine sites. The fifth through seventh were not named in the issue -- they are the same re-derivation class, and leaving them would have made the acceptance criterion false. _computePathPrefix keeps its isGlobal boolean API, so the projection is centralized rather than eliminated. resolveScope and isGlobalScope share one validator, so the two surfaces cannot drift. TWO SITES DELIBERATELY NOT ROUTED: runtime-artifact-conversion's rewriteStagedSkillBodies and rewriteStagedCommandBodies. ADR-1508 fixes the direction as installer/layout -> conversion, never upward, and install-scope composes runtime-homes, so importing it into the conversion module would invert that direction. Left as-is on purpose. Behavior-preserving throughout. Each step was proven by capturing full layout and plan output -- including every kind's home field and the hashed contents of emitted files -- before and after, across both scopes for claude, codex, opencode, hermes, kimi and kilo. Byte-identical. surface.cts keeps a scope ?? 'global' default before the call because Layout.scope is optional there; isGlobalScope throws where the old inline compare returned false, and that difference would have been a placement regression. Verified via the remote runner. * fix(#2870): cover the no-config-home throw and document the strictness Two findings from the isolated adversarial review. The vscode case was implemented but untested. resolveScope throws for a runtime whose descriptor declares configHome.kind 'none', which is the design's own behavior-table row 13, but the registry sweep excluded vscode rather than asserting the throw -- so the behavior shipped with no test. The exclusion is now legitimate because the case has its own test naming the runtime in the assertion. isGlobalScope throws where the inline compare it replaced returned false. No reachable caller can deliver an out-of-union value today, but the types are not enforced at runtime, so a future caller passing an optional Layout.scope would crash rather than silently misroute. That is the better failure -- misrouting writes artifacts to the wrong place -- but it was undocumented, so the reason is now on the function. Adds the changeset the acceptance criteria require. * refactor(#2870): route the last two sites; correct the ADR-1508 claim The previous commit declined to route runtime-artifact-conversion's rewriteStagedSkillBodies and rewriteStagedCommandBodies, claiming ADR-1508's dependency direction forbade the import. That reasoning was wrong, and this commit corrects it. Two independent reviewers checked the actual import graph: runtime-artifact-conversion already imports capability-registry, command-roster, runtime-name-policy and shell-command-projection -- it depends on leaf-tier siblings today. install-scope imports only runtime-homes plus node builtins, and runtime-homes imports only node builtins, so there is no cycle at any depth. ADR-1508 governs the installer/layout to conversion boundary, not a leaf-to-leaf sibling import of the same shape conversion already makes. With those two routed, every isGlobal re-derivation in the tree now goes through one owner and acceptance criterion 1 is fully met rather than partially. Nine sites, not the four the issue enumerated. Also from the review: Tests were falling through to the real process.cwd() at five local-scope call sites, which contradicts the acceptance criterion that the resolved value be assertable with no filesystem. Every one now injects a cwd. One of the five was a site the review had not spotted. bin/install.js carried two near-identical copies of the guarded resolveScope block, one in install() and one in uninstall() -- duplicated scope logic in the phase whose purpose is removing it. Extracted to one helper, and the new sites use the file's existing ternary idiom rather than the if/else that replaced it. Equivalence re-proven across both scopes for claude, codex, opencode, kilo and hermes, now including the staged skill and command body rewrites hashed per file, since those decide the literal spec-root path baked into every emitted artifact. Byte-identical. Verified via the remote runner. * fix(#2870): assert configHome portably instead of with a native separator The windows-latest node24 shard failed on two install-scope assertions. The module was right and the tests were wrong: they built their expected value with path.join, which emits \fake\home\.claude on Windows, while resolveScope normalizes separators unconditionally to /fake/home/.claude. That unconditional normalization is deliberate -- backslash paths arrive on Linux too, so normalizing via path.sep is the documented defect this repo guards against. Weakening it to make the assertion pass would have inverted the fix. Every path.join-built expectation in the suite now goes through toPosixPath from tests/helpers.cjs, which is the pattern the no-path-literal-in-assert rule's own valid-case list sanctions. It splits on the running platform's path.sep and rejoins with forward slashes, so it reverses whatever path.join produced on that same platform and the expectation is invariant everywhere. Two more call sites had the same latent problem and passed on Linux and macOS by luck; they are fixed too. This is the class of defect the remote runner structurally cannot catch -- its matrix is Linux-only, so a green pass there is not evidence of portability, and CI's Windows lane is the only place it surfaces. Verified via the remote runner. --------- Co-authored-by: sim <sim@local>
This commit is contained in:
@@ -36,6 +36,11 @@ const {
|
||||
resolveKimiHooksTomlDir,
|
||||
isRegisteredRuntimeId,
|
||||
} = require('../gsd-core/bin/lib/runtime-homes.cjs');
|
||||
// #2870: the Install Scope Module — turns a bare 'global' | 'local' scope id
|
||||
// plus a runtime into one resolved value (configHome, settingsFile,
|
||||
// consentRequired, hostPrecedenceRank) instead of the id being re-derived
|
||||
// and re-interpreted at each call site. See src/install-scope.cts.
|
||||
const { resolveScope } = require('../gsd-core/bin/lib/install-scope.cjs');
|
||||
// getDirName (runtime -> local config dir name) is relocated out of this
|
||||
// installer to the runtime-name-policy leaf (ADR-1508 / #1510 Phase 1) so the
|
||||
// conversion module's rewrite engine can consume it without importing
|
||||
@@ -545,6 +550,17 @@ try {
|
||||
// hardcoded string-equality branch) so behavior degrades CLOSED (safe), never open.
|
||||
// The live descriptor (capabilities/claude/capability.json) remains the source of
|
||||
// truth; this mirrors only the privacy-load-bearing subset. (ADR-1239 / #2086)
|
||||
//
|
||||
// #2870: NOT routed through the Install Scope Module (resolveScope,
|
||||
// src/install-scope.cts) despite that module owning per-scope settings-file
|
||||
// resolution elsewhere in this file. resolveScope's own descriptor lookup
|
||||
// goes through the SAME capability registry require this floor exists to
|
||||
// survive the failure of (see getRegistry() in install-scope.cts) — so on
|
||||
// exactly the "registry failed to load" path this constant is for,
|
||||
// resolveScope would throw too. Routing through it here would trade a
|
||||
// graceful, documented degrade for a crash in the one case this floor was
|
||||
// added to prevent. This hardcoded literal is the correct, honest answer,
|
||||
// not an un-migrated leftover.
|
||||
const FALLBACK_HOST_BEHAVIORS = Object.freeze({
|
||||
claude: Object.freeze({
|
||||
settingsFileByScope: Object.freeze({ local: 'settings.local.json', global: 'settings.json' }),
|
||||
@@ -606,6 +622,22 @@ function _hostIntegrationDispatch(runtime) {
|
||||
return dispatch || {};
|
||||
}
|
||||
|
||||
/**
|
||||
* #2870: shared install()/uninstall() scope resolution. Routes `id` through
|
||||
* the Install Scope Module (src/install-scope.cts) and degrades to `null` on
|
||||
* failure (unknown/non-installable runtime, broken registry bundle) instead
|
||||
* of throwing, so each call site's own plain-id fallback keeps working
|
||||
* exactly as it did before this migration. Both call sites previously carried
|
||||
* their own copy of this try/catch; this is the one shared copy.
|
||||
*/
|
||||
function _resolveScopeSafe(id, runtime) {
|
||||
try {
|
||||
return resolveScope({ id, runtime });
|
||||
} catch (_) {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the ACTUAL on-disk skills-install directory for a runtime, honoring a
|
||||
* skills-kind `home` override (ADR-1239 upgrade 3 / #2088: e.g. Codex skills ->
|
||||
@@ -8191,7 +8223,16 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) {
|
||||
} catch {}
|
||||
|
||||
// 1. Remove GSD commands/skills (layout-driven)
|
||||
const scope = isGlobal ? 'global' : 'local';
|
||||
// #2870: scope id resolved ONCE here and reused below (was two independent
|
||||
// isGlobal-derived re-derivations). Routed through the Install Scope
|
||||
// Module (src/install-scope.cts) when the capability registry is
|
||||
// available; degrades to the plain id on failure (unknown/non-installable
|
||||
// runtime, broken bundle) so this function's scope-id uses — which never
|
||||
// depended on registry availability before this migration — keep working
|
||||
// exactly as they did pre-migration.
|
||||
const _uninstallScopeId = isGlobal ? 'global' : 'local';
|
||||
const _resolvedUninstallScope = _resolveScopeSafe(_uninstallScopeId, runtime);
|
||||
const scope = _resolvedUninstallScope ? _resolvedUninstallScope.id : _uninstallScopeId;
|
||||
// ADR-1239 / #2086: drive uninstall through the public Host-Integration Interface.
|
||||
// Fail-open to the engine directly if the composed-registry adapter can't load.
|
||||
const _uninstallAdapter = _runtimeAdapter(runtime);
|
||||
@@ -8551,7 +8592,7 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) {
|
||||
fs.rmSync(legacyDir, { recursive: true });
|
||||
removedCount++;
|
||||
console.log(` ${green}✓${reset} Removed legacy commands/gsd/`);
|
||||
const _uninstallScope = isGlobal ? 'global' : 'local';
|
||||
const _uninstallScope = scope;
|
||||
if (migrateLegacyDevPreferencesToSkill(targetDir, savedLegacyArtifacts, runtime, _uninstallScope)) {
|
||||
// Compute the actual path written so the log line is accurate per-runtime
|
||||
const _layout = resolveRuntimeArtifactLayout(runtime, targetDir, _uninstallScope);
|
||||
@@ -10152,6 +10193,19 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
};
|
||||
}
|
||||
|
||||
// #2870: scope id resolved ONCE here and reused at every use below (was 10
|
||||
// independent isGlobal-derived re-derivations). Placed AFTER the kimi
|
||||
// local-deferred early return above so that return path does no extra
|
||||
// work. Routed through the Install Scope Module (src/install-scope.cts)
|
||||
// when the capability registry is available; degrades to the plain id on
|
||||
// failure (unknown/non-installable runtime, broken bundle) so this
|
||||
// function's plain scope-id uses — which never depended on registry
|
||||
// availability before this migration — keep working exactly as they did
|
||||
// pre-migration. `_installScope` (the full resolved value, not just the
|
||||
// id) additionally backs the settingsFileByScope routing below.
|
||||
const _installScopeId = isGlobal ? 'global' : 'local';
|
||||
const _installScope = _resolveScopeSafe(_installScopeId, runtime);
|
||||
|
||||
// Reusable helper to copy hooks/lib/ (git-cmd.js + gsd-graphify-rebuild.sh).
|
||||
// Defined early so it is visible to both the main and Codex code paths.
|
||||
// `allowlist` (when non-empty) restricts copying to the named top-level entries,
|
||||
@@ -10349,7 +10403,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
const codexPreInstallAgentContents = new Map();
|
||||
let codexPreInstallVersionBytes = null;
|
||||
if (_hostBehaviors(runtime).tomlConfigInstall && !isMinimalMode(_effectiveInstallMode)) {
|
||||
const _preSkillsDir = _resolveSkillsRootDir(runtime, targetDir, isGlobal ? 'global' : 'local');
|
||||
const _preSkillsDir = _resolveSkillsRootDir(runtime, targetDir, _installScopeId);
|
||||
if (fs.existsSync(_preSkillsDir)) {
|
||||
for (const entry of fs.readdirSync(_preSkillsDir, { withFileTypes: true })) {
|
||||
if (entry.isDirectory() && entry.name.startsWith('gsd-')) {
|
||||
@@ -10405,7 +10459,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
const _codexPreConfigRollback = !_hostBehaviors(runtime).tomlConfigInstall || isMinimalMode(_effectiveInstallMode) ? null : () => {
|
||||
rollbackInstallerMigrations();
|
||||
// skills/gsd-* — pass 1: restore snapshot entries (may be absent if deleted mid-install).
|
||||
const _earlySkillsDir = _resolveSkillsRootDir(runtime, targetDir, isGlobal ? 'global' : 'local');
|
||||
const _earlySkillsDir = _resolveSkillsRootDir(runtime, targetDir, _installScopeId);
|
||||
for (const skillName of codexPreInstallSkillNames) {
|
||||
const skillDirPath = path.join(_earlySkillsDir, skillName);
|
||||
const fileMap = codexPreInstallSkillContents.get(skillName);
|
||||
@@ -10503,7 +10557,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
installerMigrationResult = runInstallerMigrations({
|
||||
configDir: targetDir,
|
||||
runtime,
|
||||
scope: isGlobal ? 'global' : 'local',
|
||||
scope: _installScopeId,
|
||||
migrations: options.installerMigrations,
|
||||
baselineScan: true,
|
||||
});
|
||||
@@ -10636,7 +10690,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
|
||||
if (_isSkillsRuntime) {
|
||||
// Layout-driven install for skills-based runtimes (full and minimal modes)
|
||||
const scope = isGlobal ? 'global' : 'local';
|
||||
const scope = _installScopeId;
|
||||
// ADR-1239 upgrade 3 / #2088: a kind may declare an alternate install `home`
|
||||
// (e.g. Codex skills -> $HOME/.agents/skills) instead of the runtime's normal
|
||||
// configDir. Resolve the ACTUAL on-disk skills root here, descriptor-driven
|
||||
@@ -11566,7 +11620,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
}
|
||||
|
||||
// Write file manifest for future modification detection
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: isGlobal ? 'global' : 'local' });
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: _installScopeId });
|
||||
console.log(` ${green}✓${reset} Wrote file manifest (${MANIFEST_NAME})`);
|
||||
|
||||
// Report any backed-up local patches
|
||||
@@ -11717,7 +11771,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
// (copyCommandsAsCodexSkills removes pre-existing gsd-* dirs before re-writing)
|
||||
// are restored even when they are absent from disk at rollback time (#3245 CR).
|
||||
// • Dirs that did not pre-exist: remove entirely.
|
||||
const _rollbackSkillsDir = _resolveSkillsRootDir(runtime, targetDir, isGlobal ? 'global' : 'local');
|
||||
const _rollbackSkillsDir = _resolveSkillsRootDir(runtime, targetDir, _installScopeId);
|
||||
// Pass 1 — restore snapshot entries (may be absent from disk if deleted mid-install).
|
||||
for (const skillName of codexPreInstallSkillNames) {
|
||||
const skillDirPath = path.join(_rollbackSkillsDir, skillName);
|
||||
@@ -11832,7 +11886,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
// Re-write the manifest now that .toml agent files exist on disk.
|
||||
// The initial writeManifest call (before Codex config generation) could
|
||||
// not include agents/gsd-*.toml because those files did not yet exist.
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: isGlobal ? 'global' : 'local' });
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: _installScopeId });
|
||||
} else {
|
||||
console.log(` ${dim}↳${reset} Skipping Codex agent config generation (minimal install)`);
|
||||
}
|
||||
@@ -12120,7 +12174,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
// manifest-tracked (verified) — uninstall removes them explicitly via
|
||||
// removeCursorHooksJson + its script list, and reconcile is idempotent.
|
||||
// The re-run is retained for parity with the settings.json install path.
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: isGlobal ? 'global' : 'local' });
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: _installScopeId });
|
||||
persistActiveProfileMarker();
|
||||
return { settingsPath: null, settings: null, statuslineCommand: null, updateBannerCommand: null, runtime, configDir: targetDir };
|
||||
}
|
||||
@@ -12207,7 +12261,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
// explicitly via removeWindsurfHooksJson, and reconcileWindsurfHooksJson
|
||||
// is idempotent on repeated installs, so manifest tracking isn't needed
|
||||
// for correctness here.
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: isGlobal ? 'global' : 'local' });
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: _installScopeId });
|
||||
}
|
||||
|
||||
persistActiveProfileMarker();
|
||||
@@ -12221,7 +12275,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
writeClineArtifacts(targetDir, isGlobal);
|
||||
// Re-run the manifest pass: these artifacts are written *after* the earlier
|
||||
// writeManifest() call, so a second pass is needed to hash-track them.
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: isGlobal ? 'global' : 'local' });
|
||||
writeManifest(targetDir, runtime, { mode: _effectiveInstallMode, scope: _installScopeId });
|
||||
persistActiveProfileMarker();
|
||||
return { settingsPath: null, settings: null, statuslineCommand: null, updateBannerCommand: null, runtime, configDir: targetDir };
|
||||
}
|
||||
@@ -12236,10 +12290,24 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) {
|
||||
// #338: local Claude installs write to settings.local.json (Claude Code's per-user/gitignored slot)
|
||||
// so engineer-specific absolute paths (Node binary, home dir) never land in the repo-shared
|
||||
// settings.json. Global installs and all other runtimes continue to use settings.json.
|
||||
// #2870: the CURRENT scope's settings filename is sourced from the Install
|
||||
// Scope Module (_installScope.settingsFile, resolveScope's per-scope field)
|
||||
// instead of indexing _scopedSettings by hand. _scopedSettings itself is
|
||||
// retained unchanged as the #338-privacy fail-safe path: _hostBehaviors
|
||||
// already degrades to FALLBACK_HOST_BEHAVIORS (see that constant's comment
|
||||
// above) when the registry fails to load, whereas resolveScope's registry
|
||||
// lookup throws in that same scenario (_installScope is null when it did).
|
||||
// Falling back to _scopedSettings[_installScopeId] there — and keeping the
|
||||
// non-local-claude branch's expression untouched — means this is
|
||||
// byte-identical to the pre-migration computation in every case, including
|
||||
// the broken-registry fail-safe floor.
|
||||
const _scopedSettings = _hostBehaviors(runtime).settingsFileByScope || null;
|
||||
const isLocalClaude = (!isGlobal && !!(_scopedSettings && _scopedSettings.local));
|
||||
const _currentScopeSettingsFile = _installScope
|
||||
? _installScope.settingsFile
|
||||
: (_scopedSettings ? (_scopedSettings[_installScopeId] ?? null) : null);
|
||||
const isLocalClaude = (!isGlobal && !!_currentScopeSettingsFile);
|
||||
const settingsFileName = isLocalClaude
|
||||
? _scopedSettings.local
|
||||
? _currentScopeSettingsFile
|
||||
: ((_scopedSettings && _scopedSettings.global) || 'settings.json');
|
||||
// ADR-1239 Phase B write-confinement: the descriptor-sourced settings filename
|
||||
// must resolve under targetDir (this path also drives a recursive mkdirSync).
|
||||
|
||||
Reference in New Issue
Block a user