From 3f349e551dac3b84571cbe89671c5d8aeed1ef04 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 7 Aug 2026 18:53:25 -0400 Subject: [PATCH] fix(#3024): route sync-skills through the shipped gsd-tools instead of an unshipped install.js (#3195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#3024): sync-skills workflow uses gsd-tools query skills-root instead of unshipped install.js The sync-skills workflow Step 2 shelled out to gsd-core/bin/install.js --skills-root, but install.js is not shipped in installed trees (only in the npm tarball root bin/). Every /gsd-update --sync invocation failed with MODULE_NOT_FOUND. Fix: added 'gsd-tools query skills-root ' subcommand (gsd-tools IS shipped) that calls the same getGlobalSkillsBase function install.js used. Updated the workflow to call gsd_run query skills-root instead of the dead install.js path. Also documented the #3025 verbatim-cp limitation in Step 5 with a workaround. * test(#3024): failing-first guards for the three defects in the adopted fix The cherry-picked commit came from an aborted run that never executed its own tests. Its raw-path assertion fails as written, which is the clearest evidence the work never reached verification. Covers: - --raw must emit a bare path, not JSON (output() takes a third rawValue arg that routeSkillsRoot omits, so the raw branch never fires) - an unknown, empty, whitespace, traversing, or metacharacter-bearing runtime must be rejected, not silently resolved to claude's skills root - sync-skills.md must contain zero references to the unshipped install.js, including the guard's remediation text — the issue's second reported defect - parity across every runtime in the registry, not three hardcoded ones, so the two entry points cannot drift Also converts the adopted tests off a hand-rolled spawnSync onto the bounded process seam, per CONTRIBUTING. Fails before the fix. Verified via the remote runner. * fix(#3024): make the skills-root query actually work and reach non-Claude runtimes The cherry-picked commit never ran its own tests. Six defects, all fixed here. --raw was ignored: output() is output(result, raw, rawValue) and the third argument was omitted, so the raw branch never fired and the workflow captured a JSON blob as SRC_SKILLS_ROOT. Every downstream cp -r then resolved against a nonexistent path — the command would have shipped still broken. An unknown runtime silently resolved to claude's skills root, because getGlobalSkillsBase falls back rather than returning null, leaving the existing === null guard dead. The runtime id is now validated at the CLI boundary against the shipped registry, so a typo'd --from/--to fails instead of reading from or writing into the wrong runtime's tree. getGlobalSkillsBase('vscode') threw a raw TypeError. vscode is non-installable by descriptor, so it has no skills root — null is the answer, not a crash. The resolver now short-circuits configHome.kind 'none', which also fixes the same latent crash in install.js --skills-root vscode. Every caller already gates on === null. sync-skills.md used gsd_run WITHOUT the canonical launcher preamble, so gsd_run was undefined on non-Claude runtimes — the fix would have been dead in exactly the place the original bug bit. Preamble propagated via sync-runtime-launcher. Also registers skills-root in TOP_LEVEL_USAGE (the help/dispatch parity guard caught it), removes the last two install.js references including the guard's remediation text (the issue's second reported defect), and updates the stale assertion that still described the removed contract. Verified on the remote runner. * fix(#3024): align the documented runtime list with the registry and gate both entry points Isolated review returned BLOCK on two findings. The workflow's Supported-runtimes list and its --to all expansion named grok and gemini, neither of which is a registered runtime. Once this branch added validation, --to all — a documented first-class feature — aborted. The list was hand-copied prose shadowing the registry, so correcting it alone would drift again; a parity assertion now fails in BOTH directions if the doc and the registry disagree. vscode is excluded by name: it is installSurface 'none', so syncing skills to it is meaningless and would abort. bin/install.js --skills-root reached getGlobalSkillsBase with no own-property gate, so --skills-root __proto__ silently resolved to claude's skills root. This branch had just hardened the OTHER entry point to the same function; leaving one of two parallel surfaces open is the same divergence class as the first finding. Both now call one shared isRegisteredRuntimeId() rather than a copied check, and the parity test covers the hostile ids so the two can never disagree again. Also guards the workflow's root resolution: neither command substitution checked its exit status and only the source had an existence guard, so a failed destination resolution left DEST_ROOT empty and turned rm -rf "$DEST_ROOT/$SKILL" into an absolute path at filesystem root. Both resolutions are now checked, and Step 5 requires both roots to be non-empty and absolute before any destructive command. Verified on the remote runner. * test(#3024): anchor the runtime-list parity extractor to the list span The extractor captured (.+) to end of line, so it swallowed the em-dash prose that explains the vscode exclusion — and that sentence contains backticked `runtimes` and `null`, which is where the three phantom ids came from. The documented list was correct; the test was reading its own explanation back as data. Anchored to the id-list span. Both directions still fail as intended: proven by injecting a bogus id and by removing a registered one. * test(#3024): anchor the --to all extractor and fail loudly on empty captures The workflow has three TO_RUNTIMES= assignments and the regex matched the first one — an empty array initializer at line 28 — so the extractor captured nothing and the assertion diffed [] against 18 ids as if that were data. That is the same failure twice, so the fix is the general one: every extractor in this test now asserts it captured a plausible list before comparing, naming which extractor found nothing and what it was looking for. An extractor that silently yields [] is a confident wrong answer, and a parity guard that reports it as a data mismatch teaches the reader to loosen the assertion. Verified against the real workflow and against doctored copies with each target construct removed, plus both teeth directions. * fix(#3024): merge duplicate process-seam import after rebase The rebase applied cleanly but left runNode declared twice: next had gained its own import of the seam while this branch added one carrying OUTCOME. A clean rebase is not a correct one — the file no longer parsed. Merged into a single import providing both. * fix(#3024): bind DEST_ROOT per destination instead of a dangling map Step 2 stored each destination's root into DEST_SKILLS_ROOTS, which nothing ever read, while Steps 3 and 5 used a scalar DEST_ROOT that nothing ever assigned. The array was also never declare -A'd, so on bash 3.2 — macOS system bash, which this repo supports — every destination collapsed onto index 0. The absolute-path guard added earlier was the only thing standing between that and rm -rf "/$SKILL"; it turned a silent disaster into a hard stop, but the feature still could not complete. Each destination now binds its own DEST_ROOT where it is used, and the unread map is gone rather than replaced. Step 2 keeps eager validation, so a bad runtime id in a multi-destination --to aborts before any destination is written rather than after some already have been. Verified on bash 3.2 with a two-destination run binding distinct roots, and with a bad id aborting before any destructive call. * fix(#3024): restore grok support broken by the registry gate The registry gate added earlier rejected grok, and that was my error. I confirmed grok was absent from the capability registry and concluded the hardcoded branch was dead — without checking what it resolved to. It resolves to ~/.agents/skills, a real grok-specific path, exactly as the pre-fix workflow documented ('grok uses the ~/.agents layout'), and there is a support discussion doc for it. So a working, documented runtime silently lost --skills-root and sync-skills support as a side effect of prototype-pollution hardening — and the parity test I added locked that in as correct. gemini is the one that really was dead: it fell through to CLAUDE's skills root, so rejecting it is right and it stays rejected, as do bogus ids, __proto__, empty, whitespace and traversal. The validator's real question is 'does this id have a genuine runtime-specific resolution', not 'is it in the registry map'. Registry membership was a proxy that happened to miss grok. Legacy non-registry runtimes with dedicated resolution branches are now a named, documented set; enumerating every hardcoded branch in getGlobalConfigDir against the registry confirms grok is the only one. The new tests assert grok resolves UNDER .agents and specifically not to claude's root. Allow-listing an id proves nothing about whether it resolves correctly — that assertion is what would have caught my mistake. Also uses the shared PROBE_TIMEOUT_MS instead of a duplicate literal, and guards Step 3's DEST_ROOT re-resolution, which contradicted the file's own stated guarantee. Verified on the remote runner. * test(#3024): guard against LEGACY_NON_REGISTRY_RUNTIME_IDS drifting The named legacy set is a second hand-maintained proxy for the same predicate the registry check got wrong — 'does this id resolve runtime-specifically'. Nothing stopped a third hardcoded branch being added to getGlobalConfigDir without updating the Set, reproducing the exact class of bug that broke grok. Production stays explicit and greppable; the test derives the truth instead. It resolves a sentinel id to learn the generic fallback, classifies every candidate against it, and fails in both directions — an id resolving runtime-specifically that is in neither the registry nor the Set, or a Set entry that no longer earns its exemption. The failure message names the remedy. Confirms grok resolves runtime-specifically and gemini does not, which is the distinction the original registry check could not see. Also reverts the shared-timeout swap: SKILLS_ROOT_PROBE_TIMEOUT_MS is pre-existing on next and arrived by rebase, so changing it here was scope creep into another issue's territory. Verified on the remote runner. * chore(#3024): backfill changeset PR number --------- Co-authored-by: sim --- .changeset/daring-birds-howl.md | 5 + bin/install.js | 15 +- gsd-core/bin/gsd-tools.cjs | 37 +- gsd-core/workflows/sync-skills.md | 60 ++- src/runtime-homes.cts | 100 +++- ...sync-skills-runtime-launcher-preamble.json | 8 + tests/install.test.cjs | 491 +++++++++++++++++- ...time-homes-legacy-ids-drift-guard.test.cjs | 194 +++++++ 8 files changed, 892 insertions(+), 18 deletions(-) create mode 100644 .changeset/daring-birds-howl.md create mode 100644 tests/emitted-drift-acks/3024-sync-skills-runtime-launcher-preamble.json create mode 100644 tests/runtime-homes-legacy-ids-drift-guard.test.cjs diff --git a/.changeset/daring-birds-howl.md b/.changeset/daring-birds-howl.md new file mode 100644 index 000000000..d8e82bd00 --- /dev/null +++ b/.changeset/daring-birds-howl.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3195 +--- +**`/gsd-update --sync` no longer fails with MODULE_NOT_FOUND** — the sync-skills workflow shelled out to `gsd-core/bin/install.js`, which the installer never copies. Now uses `gsd-tools query skills-root` (which IS shipped) to resolve skills roots. (#3024) diff --git a/bin/install.js b/bin/install.js index b68b1c44d..750ebcde4 100755 --- a/bin/install.js +++ b/bin/install.js @@ -34,6 +34,7 @@ const { getGlobalConfigDir, getGlobalSkillsBase, resolveKimiHooksTomlDir, + isRegisteredRuntimeId, } = require('../gsd-core/bin/lib/runtime-homes.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 @@ -13662,7 +13663,19 @@ if (require.main === module && !process.env.GSD_TEST_MODE) { console.error('Usage: node install.js --skills-root '); process.exit(1); } - const skillsRoot = getGlobalSkillsBase(runtimeArg); + // #3024: validate the runtime id against the shipped capability registry + // BEFORE resolving anything. getGlobalSkillsBase's bare `runtimes[runtime]` + // lookup falls through the prototype chain to claude's skills root for an + // unregistered/hostile id (`__proto__`, `constructor`, `prototype`, …) + // instead of failing loudly. isRegisteredRuntimeId is the SAME validator + // gsd-tools' `routeSkillsRoot` calls, so this entry point and the shipped + // `gsd-tools query skills-root` entry point can never diverge on which + // runtime ids they accept. + if (!isRegisteredRuntimeId(runtimeArg)) { + console.error(`Unknown runtime "${runtimeArg}" — must be a registered runtime id`); + process.exit(1); + } + const skillsRoot = getGlobalSkillsBase(runtimeArg.trim()); if (skillsRoot === null) { console.error(`${runtimeArg} does not use a skills directory`); process.exit(1); diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 8600b4a70..b06c604de 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -264,6 +264,9 @@ const { resolveActiveWorkstream, applyResolvedWorkstreamEnv } = require('./lib/a const state = require('./lib/state.cjs'); const phase = require('./lib/phase.cjs'); const roadmap = require('./lib/roadmap.cjs'); +// #3024: resolve skills root for the sync-skills workflow (install.js is not +// shipped in installed trees; gsd-tools IS shipped, so the workflow calls this). +const { getGlobalSkillsBase, isRegisteredRuntimeId } = require('./lib/runtime-homes.cjs'); // #1561 — assumption-delta advisory checkpoint detector (pure function). const { detectAssumptionDelta } = require('./lib/assumption-delta.cjs'); const verify = require('./lib/verify.cjs'); @@ -1044,6 +1047,37 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load commands.cmdCurrentTimestamp(args[1] || 'full', raw); } + function routeSkillsRoot({ args, raw, error }) { + // #3024: resolve the global skills base directory for a runtime. + // The sync-skills workflow previously shelled out to install.js --skills-root, + // but install.js is not shipped in installed trees. gsd-tools IS shipped, so + // the workflow now calls `gsd-tools query skills-root ` instead. + const runtime = args[1]; + if (!runtime) { + error('Usage: gsd-tools query skills-root '); + } + // Defect B (#3024): validate the runtime id against the shipped capability + // registry's canonical runtime set BEFORE resolving anything. + // getGlobalSkillsBase falls through getGlobalConfigDir's unknown-runtime + // branch to claude's skills root for ANY id it doesn't recognize, so an + // unknown, empty/whitespace-only, path-traversal, or shell-metacharacter + // runtime arg would otherwise silently resolve to claude's path instead of + // failing loudly. isRegisteredRuntimeId does an own-property lookup (not a + // bare index), rejecting `__proto__`/`constructor`/`prototype` runtime + // ids, and is the SAME validator install.js's `--skills-root` entry point + // calls, so the two shipped entry points can never diverge on which + // runtime ids they accept. + if (!isRegisteredRuntimeId(runtime)) { + error(`Unknown runtime "${runtime}" — must be a registered runtime id`); + } + const trimmedRuntime = typeof runtime === 'string' ? runtime.trim() : ''; + const skillsRoot = getGlobalSkillsBase(trimmedRuntime); + if (skillsRoot === null) { + error(`No skills root found for runtime "${trimmedRuntime}"`); + } + output({ skills_root: skillsRoot }, raw, skillsRoot); + } + function routeProjectInstructionFile({ args, cwd, raw, error }) { // #1529: pure runtime→filename projection. Backs the // `gsd_run query project-instruction-file --runtime ` call in @@ -3345,6 +3379,7 @@ const HOST_COMMAND_ROUTERS = { 'user-story': routeUserStory, 'drift-guard': routeDriftGuard, 'windows': routeWindows, + 'skills-root': routeSkillsRoot, }; // Returns true when consumed (suppress "Unknown command"), false to fall @@ -3561,7 +3596,7 @@ const TOP_LEVEL_USAGE = 'Usage: gsd-tools [args] [--raw] [--pick /dev/null 2>&1; then GSD_TOOLS="$(command -v gsd-tools)"; gsd_run() { "$GSD_TOOLS" "$@"; }; elif [ -f "${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; else echo "ERROR: gsd-tools.cjs not found at $GSD_TOOLS and gsd-tools is not on PATH. Run: npx -y @opengsd/gsd-core@latest --claude --local" >&2; exit 1; fi; if [ -n "${CLAUDE_ENV_FILE:-}" ] && [ -n "${GSD_TOOLS:-}" ]; then printf "export PATH='%s':\"\$PATH\"\n" "${GSD_TOOLS%/*}" >> "$CLAUDE_ENV_FILE" 2>/dev/null || true; fi +SRC_SKILLS_ROOT=$(gsd_run query skills-root "$FROM_RUNTIME" --raw) +if [ $? -ne 0 ] || [ -z "$SRC_SKILLS_ROOT" ]; then + echo "error: failed to resolve skills root for runtime '$FROM_RUNTIME' (gsd_run query skills-root $FROM_RUNTIME --raw)" >&2 + exit 1 +fi for DEST_RUNTIME in "${TO_RUNTIMES[@]}"; do - DEST_SKILLS_ROOTS["$DEST_RUNTIME"]=$(node "$INSTALL_JS" --skills-root "$DEST_RUNTIME") + RESOLVED_DEST_ROOT=$(gsd_run query skills-root "$DEST_RUNTIME" --raw) + if [ $? -ne 0 ] || [ -z "$RESOLVED_DEST_ROOT" ]; then + echo "error: failed to resolve skills root for runtime '$DEST_RUNTIME' (gsd_run query skills-root $DEST_RUNTIME --raw)" >&2 + exit 1 + fi done ``` +This loop validates every destination in `TO_RUNTIMES` up front — a bad runtime id anywhere in a multi-destination `--to` aborts here, before Step 3 or Step 5 touch anything. The resolved value itself is not retained: each of Steps 3 and 5 re-resolves `DEST_ROOT` for the specific `$DEST_RUNTIME` it is currently processing (see those steps), so `$DEST_ROOT` is always unambiguously scoped to one destination and never threaded through a shared array. + **Guard:** If the source skills root does not exist, print: ``` error: source skills root not found: Is GSD installed globally for the '' runtime? - Run: node ~/.claude/gsd-core/bin/install.js --global -- + Run: npx -y @opengsd/gsd-core@latest --global -- ``` Then exit. +**Guard:** If resolving the skills root for the source OR any destination runtime fails (`gsd_run query skills-root --raw` exits non-zero or prints nothing — see Step 2's bash), print: +``` +error: failed to resolve skills root for runtime '' + command: gsd_run query skills-root --raw + Is '' a registered runtime id? See supported runtime names above. +``` +Then exit. Never proceed to Step 3 or Step 5 with an empty or unresolved root — an empty `$DEST_ROOT` turns `rm -rf "$DEST_ROOT/$SKILL"` into `rm -rf "/$SKILL"`. + **Guard:** If `--to` contains the same runtime as `--from`, skip that destination silently. --- @@ -87,6 +101,16 @@ Then exit. For each destination runtime: ```bash +# Bind the destination root for this iteration's destination runtime. Already +# validated to resolve successfully in Step 2's eager-validation loop; +# re-resolving here (rather than reading back a shared array) keeps this +# value unambiguously scoped to the destination currently being processed. +DEST_ROOT=$(gsd_run query skills-root "$DEST_RUNTIME" --raw) +if [ $? -ne 0 ] || [ -z "$DEST_ROOT" ]; then + echo "error: failed to resolve skills root for runtime '$DEST_RUNTIME' (gsd_run query skills-root $DEST_RUNTIME --raw)" >&2 + exit 1 +fi + # List gsd-* subdirectories in source SRC_SKILLS=$(ls -1 "$SRC_SKILLS_ROOT" 2>/dev/null | grep '^gsd-') @@ -142,8 +166,22 @@ If `--dry-run` (or no flag): skip this step entirely and exit after printing the For each destination with changes: ```bash +# Bind DEST_ROOT for this iteration's destination (see Step 3's identical +# re-resolution note — Step 2 already validated this resolves successfully). +DEST_ROOT=$(gsd_run query skills-root "$DEST_RUNTIME" --raw) + +[[ "$SRC_SKILLS_ROOT" == /* ]] || { echo "error: SRC_SKILLS_ROOT is empty or not absolute: '$SRC_SKILLS_ROOT'" >&2; exit 1; } +[[ "$DEST_ROOT" == /* ]] || { echo "error: DEST_ROOT is empty or not absolute: '$DEST_ROOT'" >&2; exit 1; } + mkdir -p "$DEST_ROOT" +# #3025: verbatim cp -r copies the SOURCE runtime's converted skill form, which +# corrupts skills for destinations that need a different conversion (e.g. Claude +# SKILL.md → Codex TOML agent). Until the installer exposes a per-skill conversion +# CLI, sync is limited to runtime pairs that share the same skill format. +# Run `gsd install -- --local` to get correctly converted skills +# for a destination that uses a different format. + for SKILL in $CREATE_LIST $UPDATE_LIST; do rm -rf "$DEST_ROOT/$SKILL" cp -r "$SRC_SKILLS_ROOT/$SKILL" "$DEST_ROOT/$SKILL" diff --git a/src/runtime-homes.cts b/src/runtime-homes.cts index 97d10b7e7..e8f37818e 100644 --- a/src/runtime-homes.cts +++ b/src/runtime-homes.cts @@ -141,11 +141,28 @@ interface GenericAgentsRootDescriptor { skillsHome?: ConfigHomeDescriptor; } +/** + * #2103: a runtime with NO file-projected config directory at all (e.g. + * vscode — Marketplace/VSIX extension, `installSurface: 'none'`). There is + * no directory to resolve, so this descriptor kind is deliberately excluded + * from `resolveConfigHomeFromDescriptor`'s directory-resolving switch (see + * that function's 'none' case, which throws rather than silently falling + * through). Callers that need a nullable result (e.g. `getGlobalSkillsBase`) + * must check `configHome.kind === 'none'` themselves before resolving. + */ +interface NoneDescriptor { + kind: 'none'; + name: string; + env: string[]; + skillsHome?: ConfigHomeDescriptor; +} + type ConfigHomeDescriptor = | DotHomeDescriptor | DotHomeNestedDescriptor | XdgDescriptor - | GenericAgentsRootDescriptor; + | GenericAgentsRootDescriptor + | NoneDescriptor; interface RuntimeArtifactKindDescriptor { kind: string; @@ -177,6 +194,63 @@ function getRegistry(): { runtimes: Record = new Set(['grok']); + +/** + * True when `runtime` is a real runtime id with a genuine, runtime-specific + * resolution path — either a registered id in the capability registry + * (`capability-registry.cjs`'s `runtimes` object) or one of the small, + * explicitly named `LEGACY_NON_REGISTRY_RUNTIME_IDS` set (currently just + * `grok`) that resolves via a dedicated hardcoded branch instead of a + * registry descriptor. + * + * Guarded with an own-property lookup — never a bare index — so a + * prototype-chain id (`__proto__`, `constructor`, `prototype`, `toString`, + * …) can never resolve to an inherited value and be mistaken for a real + * entry. Without this, `getGlobalSkillsBase`/`getGlobalConfigDir`'s bare + * `runtimes[runtime]` lookups fall through the prototype chain for those + * ids, find no usable descriptor, and silently resolve to claude's + * fallback path instead of failing loudly (#3024). + * + * Single shared validator for every `--skills-root` entry point + * (`gsd-tools query skills-root`'s `routeSkillsRoot`, `install.js + * --skills-root`) so the two surfaces can never diverge on which runtime + * ids they accept. + */ +export function isRegisteredRuntimeId(runtime: unknown): boolean { + if (typeof runtime !== 'string') return false; + const trimmed = runtime.trim(); + if (!trimmed) return false; + if (Object.prototype.hasOwnProperty.call(getRegistry().runtimes, trimmed)) return true; + return LEGACY_NON_REGISTRY_RUNTIME_IDS.has(trimmed); +} + /** * Resolve a configHome descriptor to an absolute directory path. * @@ -272,6 +346,20 @@ export function resolveConfigHomeFromDescriptor( // fallback: first probe candidate return expandTilde(configHome.probe[0], home); } + + case 'none': { + // #2103: no file-projected config directory exists for this runtime + // (e.g. vscode). Previously this kind had no matching case, so the + // switch fell through and implicitly returned `undefined` — which + // then crashed a downstream `path.join(undefined, ...)` with a + // cryptic `TypeError [ERR_INVALID_ARG_TYPE]` far from the real cause. + // Throwing here makes the failure mode explicit; callers that need a + // nullable result (getGlobalSkillsBase) check `configHome.kind` and + // short-circuit BEFORE ever reaching this function. + throw new Error( + `Runtime "${configHome.name}" has no config-home directory (configHome.kind === "none")`, + ); + } } } @@ -483,6 +571,16 @@ export function resolveSkillsBaseFromDescriptor( export function getGlobalSkillsBase(runtime: string): string | null { const runtimeEntry = getRegistry().runtimes[runtime]; const descriptor = runtimeEntry?.runtime; + // #2103: a runtime with `configHome.kind === 'none'` (e.g. vscode — + // Marketplace/VSIX extension, installSurface:'none') has no file-projected + // config directory at all, and therefore no skills root. Short-circuit to + // null BEFORE falling through to getGlobalConfigDir below, which would + // otherwise throw resolving a 'none' configHome (see + // resolveConfigHomeFromDescriptor's 'none' case). null is the correct + // answer here, not a crash — the `=== null` guard at every call site + // (bin/install.js --skills-root, gsd-tools routeSkillsRoot) already + // handles it as "this runtime does not use a skills directory". + if (descriptor?.configHome?.kind === 'none') return null; const globalSkillsKind = descriptor?.artifactLayout?.global?.find((entry) => entry.kind === 'skills'); // ADR-1239 upgrade 3 (#2088): honor a skills-kind `home` override (e.g. Codex // → $HOME/.agents/skills, independent of $CODEX_HOME) so the reported skills diff --git a/tests/emitted-drift-acks/3024-sync-skills-runtime-launcher-preamble.json b/tests/emitted-drift-acks/3024-sync-skills-runtime-launcher-preamble.json new file mode 100644 index 000000000..0c2ab1bab --- /dev/null +++ b/tests/emitted-drift-acks/3024-sync-skills-runtime-launcher-preamble.json @@ -0,0 +1,8 @@ +{ + "version": 1, + "paths": { + "sync-skills.md": { + "reason": "#3024: Step 2 now resolves skills roots via `gsd_run query skills-root ` instead of shelling out to the unshipped `install.js --skills-root`. Because the workflow now calls `gsd_run`, it must carry the canonical runtime-launcher preamble (`node scripts/sync-runtime-launcher.cjs`, per runtime-launcher-parity.test.cjs) so `gsd_run` resolves on every non-Claude runtime, not just Claude Code — without it the fix would be dead exactly where the original #3024 bug bit. The single-line preamble (~4.4KB, one bash line with a resolver arm per supported runtime home) accounts for essentially all of the growth (6,125 -> 10,928 bytes); the remainder is the reworded Step 2 lead-in and error-guidance text that no longer references the unshipped install.js entry point (defect C). Follow-up hardening (still #3024): neither `gsd_run query skills-root` call in Step 2 checked its exit status or for an empty result, so an unregistered/invalid runtime id silently produced an empty `$DEST_ROOT`, which Step 5 then fed into `rm -rf \"$DEST_ROOT/$SKILL\"` (an absolute-root deletion). Step 2's bash now checks exit status + non-empty for both the source and every destination resolution and aborts with a named error; a matching prose guard covers destination-resolution failure alongside the pre-existing source-not-found guard; Step 5 adds a one-line non-empty/absolute check on both `$SRC_SKILLS_ROOT` and `$DEST_ROOT` before any `rm -rf`/`cp -r`. (Separately noted but NOT fixed here, as it requires restructuring Steps 3-5 rather than a Step 2/Step 5 guard: `DEST_SKILLS_ROOTS` is populated as a keyed map but is never read back into `$DEST_ROOT` anywhere in the file, and on bash 3.2 — the macOS system `/bin/bash` — assigning to it without `declare -A` silently collapses every destination's resolved root into index `[0]`; `declare -A` itself is bash4+-only and errors on 3.2, so this is not a one-line fix.) This accounts for the additional growth (10,928 -> 12,166 bytes). Follow-up (review BLOCKER on this fix): the \"Supported runtime names\" prose list and the `--to all` TO_RUNTIMES expansion both hand-copied a runtime-id list that had drifted from the capability registry — `grok` and `gemini` were listed but are not registered runtime ids, and this branch's own-property validation gate in `routeSkillsRoot` now correctly rejects them, so `--to all` (and any explicit `--to grok`/`--to gemini`) aborted. Both lists are corrected to the registry's 19 runtime ids minus `vscode` (deliberately excluded and named in prose: `installSurface: 'none'`, `getGlobalSkillsBase('vscode')` returns `null`, so a sync to/from it cannot succeed) — 18 ids. A parity test (`tests/install.test.cjs`) now fails if the documented list and the registry disagree in either direction. This accounts for the growth (12,166 -> 12,510 bytes). Follow-up (this fix, the previously-deferred `DEST_SKILLS_ROOTS` defect noted above): that associative-array assignment is dropped entirely — it was never read back anywhere in the file and, on bash 3.2, silently collapsed every destination's resolved root into index `[0]`. Step 2's TO_RUNTIMES loop is kept for its eager per-destination validation (a bad runtime id anywhere in a multi-destination `--to` still aborts before any destination is touched) but no longer stores the resolved value. Steps 3 and 5 each now bind `DEST_ROOT` themselves at the top of their own bash block via `DEST_ROOT=$(gsd_run query skills-root \"$DEST_RUNTIME\" --raw)`, so every use of `$DEST_ROOT` is unambiguously scoped to the destination currently being processed in that construct, with no shared array and no bash4+ syntax. This accounts for the final growth (12,510 -> 13,480 bytes). Follow-up (delta-review BLOCKER: the previous fix's `grok`/`gemini` removal was itself wrong for `grok`): `grok` has a live, dedicated `~/.agents`-layout resolution branch in `getGlobalConfigDir` (`src/runtime-homes.cts`'s `LEGACY_NON_REGISTRY_RUNTIME_IDS`) predating the capability registry, and was documented pre-fix — removing it silently broke working, documented `--skills-root`/`sync-skills` support. `grok` is restored to both the \"Supported runtime names\" prose list and the `--to all` TO_RUNTIMES expansion, with a clause explaining why it is included despite being absent from the registry; `gemini` stays excluded (it has no dedicated branch and falls through to claude's skills root — the wrong-runtime bug this PR exists to fix). Separately, Step 3 (`ls`-based diff computation) re-resolved `DEST_ROOT` with no exit-status/empty guard, contradicting the file's own stated guarantee (\"never proceed to Step 3 or Step 5 with an empty or unresolved root\") even though Step 5 already carried that guard; Step 3 now carries the identical exit-status + non-empty check immediately after its `DEST_ROOT` resolution. This accounts for the final growth (13,480 -> 13,842 bytes)." + } + } +} diff --git a/tests/install.test.cjs b/tests/install.test.cjs index 5e9efcf6b..df4721bf3 100644 --- a/tests/install.test.cjs +++ b/tests/install.test.cjs @@ -10152,7 +10152,7 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); -const { runNode } = require('./helpers/process-seam.cjs'); +const { runNode, OUTCOME } = require('./helpers/process-seam.cjs'); const os = require('node:os'); // A single short CLI query (install.js --skills-root ) — no full @@ -10207,6 +10207,224 @@ describe('install.js --skills-root', () => { }); }); +// ── gsd-tools query skills-root (#3024) ────────────────────────────────────── + +describe('#3024: gsd-tools query skills-root', () => { + const TOOLS_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); + + // Must agree with install.js --skills-root for every runtime (same underlying + // getGlobalSkillsBase function, just a different entry point that IS shipped). + const CASES = [ + { runtime: 'claude', expected: path.join(os.homedir(), '.claude', 'skills') }, + { runtime: 'codex', expected: path.join(os.homedir(), '.agents', 'skills') }, + { runtime: 'cursor', expected: path.join(os.homedir(), '.cursor', 'skills') }, + ]; + + for (const { runtime, expected } of CASES) { + test(`resolves correct skills root for ${runtime}`, () => { + const result = runNode([TOOLS_PATH, 'query', 'skills-root', runtime, '--raw'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.strictEqual(result.exitCode, 0, `gsd-tools exited ${result.exitCode}: ${result.stderr}`); + const actual = result.stdout.trim(); + assert.strictEqual(actual, expected, `Expected ${expected}, got ${actual}`); + }); + } + + test('errors when runtime arg is missing', () => { + const result = runNode([TOOLS_PATH, 'query', 'skills-root'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.notStrictEqual(result.exitCode, 0, 'Should exit with error when runtime arg is missing'); + }); + + test('non-raw form: query skills-root claude parses as JSON with expected skills_root', () => { + const result = runNode([TOOLS_PATH, 'query', 'skills-root', 'claude'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.strictEqual(result.exitCode, 0, `gsd-tools exited ${result.exitCode}: ${result.stderr}`); + const expected = path.join(os.homedir(), '.claude', 'skills'); + const parsed = JSON.parse(result.stdout); + assert.strictEqual(parsed.skills_root, expected, `Expected ${expected}, got ${parsed.skills_root}`); + }); + + // Defect B regression: an unknown runtime must NOT silently resolve to + // claude's skills root. getGlobalSkillsBase falls back through + // getGlobalConfigDir instead of returning null, so routeSkillsRoot's + // `if (skillsRoot === null)` guard never fires for a bogus runtime id. + test('defect B: unknown runtime does not silently resolve to claude skills root', () => { + const claudeSkillsRoot = path.join(os.homedir(), '.claude', 'skills'); + const result = runNode([TOOLS_PATH, 'query', 'skills-root', 'bogus-runtime', '--raw'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.notStrictEqual(result.exitCode, 0, 'Unknown runtime must exit non-zero'); + assert.ok( + !result.stdout.includes(claudeSkillsRoot), + `stdout must not silently emit claude's skills root for an unknown runtime; got: ${result.stdout}` + ); + }); + + test('empty runtime arg exits non-zero', () => { + const result = runNode([TOOLS_PATH, 'query', 'skills-root', '', '--raw'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.notStrictEqual(result.exitCode, 0, 'Empty runtime must exit non-zero'); + }); + + test('whitespace-only runtime arg exits non-zero', () => { + const result = runNode([TOOLS_PATH, 'query', 'skills-root', ' ', '--raw'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.notStrictEqual(result.exitCode, 0, 'Whitespace-only runtime must exit non-zero'); + }); + + test('HOSTILE: path-traversal runtime arg exits non-zero and emits no path', () => { + const result = runNode([TOOLS_PATH, 'query', 'skills-root', '../../etc', '--raw'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.notStrictEqual(result.exitCode, 0, 'Path-traversal runtime must exit non-zero'); + assert.strictEqual(result.stdout.trim(), '', `stdout must not emit a path; got: ${result.stdout}`); + }); + + // HOSTILE: the seam spawns via an argv array (see process-seam.cjs), never a + // shell string, so this value can never reach a shell for interpolation. + // The assertion below confirms the command rejects the runtime value and + // produces no output — it cannot (and does not need to) prove + // shell-injection safety, because no shell is ever invoked. + test('HOSTILE: shell-metacharacter runtime arg exits non-zero with no output', () => { + const result = runNode([TOOLS_PATH, 'query', 'skills-root', 'claude; rm -rf /', '--raw'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.notStrictEqual(result.exitCode, 0, 'Shell-metacharacter runtime must exit non-zero'); + assert.strictEqual(result.stdout.trim(), '', `stdout must not emit a path; got: ${result.stdout}`); + }); + + // #3024 review BLOCKER (finding 2): install.js --skills-root had NO + // own-property gate, so a prototype-chain id silently resolved to claude's + // skills root via prototype fallthrough in getGlobalConfigDir, while + // gsd-tools' routeSkillsRoot (this same PR) rejects it. HOSTILE ids are + // included in the SAME loop as the registered runtimes so both surfaces are + // asserted to agree (or both reject) with one shared mechanism, and the + // hostile branch below additionally pins the outcome to "both reject" — + // agreement alone would not catch a defect where both sides silently + // accepted the same wrong value. + const HOSTILE_RUNTIME_IDS = ['__proto__', 'constructor', 'prototype', 'toString', '', ' ']; + + test('parity: gsd-tools query skills-root matches install.js --skills-root for every registered runtime and every hostile id', () => { + const { runtimes } = require('../gsd-core/bin/lib/capability-registry.cjs'); + const runtimeIds = Object.keys(runtimes); + assert.ok(runtimeIds.length > 0, 'capability registry must list at least one runtime'); + + for (const runtime of [...runtimeIds, ...HOSTILE_RUNTIME_IDS]) { + const isHostile = HOSTILE_RUNTIME_IDS.includes(runtime); + const toolsResult = runNode([TOOLS_PATH, 'query', 'skills-root', runtime, '--raw'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + const installResult = runNode([INSTALL_JS, '--skills-root', runtime], { + env: { ...process.env, GSD_TEST_MODE: undefined }, + }); + assert.equal(toolsResult.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly for ${JSON.stringify(runtime)}: ${JSON.stringify(toolsResult)}`); + assert.equal(installResult.outcome, OUTCOME.EXITED, `install.js did not exit cleanly for ${JSON.stringify(runtime)}: ${JSON.stringify(installResult)}`); + const toolsIsZero = toolsResult.exitCode === 0; + const installIsZero = installResult.exitCode === 0; + assert.strictEqual( + toolsIsZero, installIsZero, + `runtime ${JSON.stringify(runtime)}: exit-code agreement mismatch (gsd-tools=${toolsResult.exitCode}, install.js=${installResult.exitCode})` + ); + if (isHostile) { + assert.strictEqual(toolsIsZero, false, `HOSTILE id ${JSON.stringify(runtime)}: gsd-tools must reject, not accept`); + assert.strictEqual(installIsZero, false, `HOSTILE id ${JSON.stringify(runtime)}: install.js must reject, not accept`); + } + if (toolsIsZero && installIsZero) { + const toolsPath = String(toolsResult.stdout.trim()).replace(/\\/g, '/'); + const installPath = String(installResult.stdout.trim()).replace(/\\/g, '/'); + assert.strictEqual( + toolsPath, installPath, + `runtime ${JSON.stringify(runtime)}: gsd-tools (${toolsPath}) and install.js (${installPath}) must resolve the same skills root` + ); + } + } + }); + + // #3024 review BLOCKER (finding 2 regression pin): before the fix, + // `node bin/install.js --skills-root __proto__` printed claude's skills + // root via prototype fallthrough instead of rejecting. Pin the exact + // observable symptom, not just the exit code, so a future regression that + // reintroduces a bare `runtimes[runtime]` lookup is caught even if it + // happens to also exit non-zero for some unrelated reason. + test('HOSTILE: install.js --skills-root __proto__ does not silently resolve to claude skills root', () => { + const claudeSkillsRoot = path.join(os.homedir(), '.claude', 'skills'); + const result = runNode([INSTALL_JS, '--skills-root', '__proto__'], { + env: { ...process.env, GSD_TEST_MODE: undefined }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `install.js did not exit cleanly: ${JSON.stringify(result)}`); + assert.notStrictEqual(result.exitCode, 0, '__proto__ runtime must exit non-zero'); + assert.ok( + !result.stdout.includes(claudeSkillsRoot), + `stdout must not silently emit claude's skills root for __proto__; got: ${result.stdout}` + ); + }); + + // #3024 review BLOCKER (finding 1 regression pin): the first fix pass + // rejected `grok` outright, and a test that only asserts "grok is + // accepted" would not have caught that regression's root cause — an + // allow-list membership check proves nothing about whether the id + // actually resolves anywhere real. This asserts the REAL contract: grok + // must both be accepted AND resolve to its own dedicated `~/.agents` + // layout (see `LEGACY_NON_REGISTRY_RUNTIME_IDS` in + // `src/runtime-homes.cts`), not to claude's wrong-runtime fallback. + test('grok: accepted, and resolves under .agents — NOT claude\'s skills root (inclusion is real, not merely allow-listed)', () => { + const claudeSkillsRoot = path.join(os.homedir(), '.claude', 'skills'); + const grokSkillsRoot = path.join(os.homedir(), '.agents', 'skills'); + const result = runNode([TOOLS_PATH, 'query', 'skills-root', 'grok', '--raw'], { + env: { ...process.env, GSD_TEST_MODE: '1', GROK_AGENTS_HOME: undefined }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.strictEqual(result.exitCode, 0, `grok must be accepted; gsd-tools exited ${result.exitCode}: ${result.stderr}`); + const actual = result.stdout.trim(); + assert.strictEqual(actual, grokSkillsRoot, `Expected grok to resolve under .agents (${grokSkillsRoot}), got ${actual}`); + assert.notStrictEqual(actual, claudeSkillsRoot, "grok must NOT resolve to claude's skills root"); + }); + + // #3024 review BLOCKER (finding 1, rejection-side teeth): `gemini` must + // stay rejected, and — critically — for the RIGHT reason. It is not + // merely "unregistered" (grok is unregistered too, and is correctly + // accepted via LEGACY_NON_REGISTRY_RUNTIME_IDS); gemini has no dedicated + // resolution branch anywhere, so `getGlobalSkillsBase('gemini')` silently + // falls through to claude's wrong-runtime fallback path. The CLI-level + // assertion proves gemini is rejected; the direct-resolution assertion + // below proves WHY it must stay rejected — if gemini ever grew a real + // dedicated branch (making it grok's true peer), this second assertion + // would fail and force a conscious decision, instead of someone + // reflexively adding it to LEGACY_NON_REGISTRY_RUNTIME_IDS. + test("gemini: rejected — because its bare resolution is claude's fallback, not because it is merely unregistered", () => { + const claudeSkillsRoot = path.join(os.homedir(), '.claude', 'skills'); + const result = runNode([TOOLS_PATH, 'query', 'skills-root', 'gemini', '--raw'], { + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gsd-tools did not exit cleanly: ${JSON.stringify(result)}`); + assert.notStrictEqual(result.exitCode, 0, 'gemini must be rejected by isRegisteredRuntimeId'); + assert.strictEqual(result.stdout.trim(), '', `stdout must not emit a path for rejected gemini; got: ${result.stdout}`); + + // Prove the reason: gemini's underlying (ungated) resolution IS claude's + // wrong-runtime fallback — unlike grok's, which resolves to a real, + // distinct path (see the sibling grok test above). + const { getGlobalSkillsBase } = require('../gsd-core/bin/lib/runtime-homes.cjs'); + assert.strictEqual( + getGlobalSkillsBase('gemini'), claudeSkillsRoot, + "gemini's bare resolution must be claude's fallback path — this is the wrong-runtime bug that justifies keeping gemini rejected" + ); + }); +}); + // ── sync-skills.md workflow content ────────────────────────────────────────── describe('sync-skills.md — required behavioral specs', () => { @@ -10251,6 +10469,117 @@ describe('sync-skills.md — required behavioral specs', () => { ); }); + // #3024 review BLOCKER (finding 1 — REGRESSION): this branch's first fix + // pass hand-derived the "expected" set from the registry alone and dropped + // `grok` from both the doc list and the `--to all` expansion, on the + // mistaken assumption that "not in the registry" means "not a real + // runtime". `grok` has a live, dedicated `~/.agents`-layout resolution + // branch in `getGlobalConfigDir` (see `LEGACY_NON_REGISTRY_RUNTIME_IDS` in + // `src/runtime-homes.cts`) predating the capability registry, so removing + // it silently broke working, documented `--skills-root`/`sync-skills` + // support. `gemini`, by contrast, is correctly excluded: it has NO + // dedicated branch and falls through to claude's skills root — the + // wrong-runtime bug this whole PR exists to fix. This parity test fails + // the moment either doc list next diverges from (registry ∪ + // NAMED_LEGACY_INCLUSIONS) minus NAMED_DELIBERATE_EXCLUSIONS, in EITHER + // direction (an id in the doc but not expected, or an id expected but + // missing from the doc). + test('parity: documented runtime list matches the capability registry (minus deliberate exclusions)', () => { + content = content || readWorkflow(); + const { runtimes } = require('../gsd-core/bin/lib/capability-registry.cjs'); + const { LEGACY_NON_REGISTRY_RUNTIME_IDS } = require('../gsd-core/bin/lib/runtime-homes.cjs'); + const registryIds = Object.keys(runtimes); + assert.ok(registryIds.length > 0, 'capability registry must list at least one runtime'); + + // Named, single-source inclusion: runtime ids with a genuine dedicated + // resolution branch (see `LEGACY_NON_REGISTRY_RUNTIME_IDS` in + // `src/runtime-homes.cts`) but no capability-registry descriptor. As of + // #3024 this is `grok` only — imported directly from the same constant + // `isRegisteredRuntimeId` gates on, so this test and the validator can + // never independently drift on which legacy ids are "real". + const NAMED_LEGACY_INCLUSIONS = [...LEGACY_NON_REGISTRY_RUNTIME_IDS]; + assert.ok( + NAMED_LEGACY_INCLUSIONS.length > 0 && NAMED_LEGACY_INCLUSIONS.includes('grok'), + `expected LEGACY_NON_REGISTRY_RUNTIME_IDS to include 'grok'; got ${JSON.stringify(NAMED_LEGACY_INCLUSIONS)}` + ); + for (const included of NAMED_LEGACY_INCLUSIONS) { + assert.ok( + !registryIds.includes(included), + `named legacy inclusion "${included}" is already a registry id — remove it from LEGACY_NON_REGISTRY_RUNTIME_IDS, not from this test` + ); + } + + // Deliberate, named exclusion: vscode is `installSurface: 'none'` (#2103) + // and `getGlobalSkillsBase('vscode')` returns null, so a skills-root sync + // to/from it can never succeed. It is named as excluded in the workflow's + // own "Supported runtime names" prose, and excluded here identically, so + // this test cannot silently drift from the prose that documents it. + const NAMED_DELIBERATE_EXCLUSIONS = ['vscode']; + for (const excluded of NAMED_DELIBERATE_EXCLUSIONS) { + assert.ok( + registryIds.includes(excluded), + `deliberate exclusion "${excluded}" must itself be a real registry id (nothing to exclude otherwise)` + ); + } + const expectedIds = registryIds + .filter((id) => !NAMED_DELIBERATE_EXCLUSIONS.includes(id)) + .concat(NAMED_LEGACY_INCLUSIONS) + .sort(); + + // Anchored to the "Supported runtime names:" line's id list ONLY: capture + // stops at the " — the full capability registry runtime set" prose that + // follows the list. A file-wide backtick sweep over the rest of that + // sentence (which explains the vscode exclusion) previously also matched + // `runtimes`, `null`, and `vscode` from unrelated inline-code spans in the + // explanatory prose — this scopes extraction to the list construct itself. + const supportedLineMatch = content.match( + /\*\*Supported runtime names:\*\*\s*([^\n]*?)\s+—\s+the full capability registry runtime set/ + ); + assert.ok(supportedLineMatch, 'workflow must have a "Supported runtime names:" line ending at " — the full capability registry runtime set"'); + const docIds = [...supportedLineMatch[1].matchAll(/`([a-z0-9-]+)`/g)].map((m) => m[1]).sort(); + assert.ok( + docIds.length > 0 && docIds.every((id) => /^[a-z0-9-]+$/.test(id)), + '"Supported runtime names:" extractor matched nothing plausible: expected one or more ' + + `backtick-wrapped runtime ids (e.g. \`claude\`) in the line's id list, got ${JSON.stringify(docIds)}` + ); + + // Anchored to the `--to all` branch specifically: the file defines + // TO_RUNTIMES=() three times (an empty initializer, this branch's full + // expansion, and the explicit `--to ` branch's command + // substitution), and an unanchored match on the FIRST occurrence anywhere + // in the file silently captures the empty initializer instead (#3024 + // remote-runner regression: toAllIds resolved to [] with no assertion + // failure until the final deepStrictEqual). Locate the `--to all` marker + // first, then take the first TO_RUNTIMES=(...) assignment after it — + // which is this branch's expansion, not the empty initializer that + // precedes the marker or the `$(...)` substitution branch that follows it. + const toAllMarkerIndex = content.indexOf('"--to all"'); + assert.ok(toAllMarkerIndex > -1, 'workflow must contain a `--to all` branch marker (`"--to all"`)'); + const toAllMatch = content.slice(toAllMarkerIndex).match(/TO_RUNTIMES=\(([^)]*)\)/); + assert.ok(toAllMatch, 'workflow must define TO_RUNTIMES=(...) for `--to all` after the `--to all` marker'); + const toAllIds = toAllMatch[1].trim().split(/\s+/).filter(Boolean).sort(); + assert.ok( + toAllIds.length > 0 && toAllIds.every((id) => /^[a-z0-9-]+$/.test(id)), + '`--to all` TO_RUNTIMES extractor matched nothing plausible: expected one or more ' + + `space-separated runtime ids inside TO_RUNTIMES=(...) after the \`--to all\` marker, got ${JSON.stringify(toAllIds)}` + ); + + assert.deepStrictEqual( + docIds, expectedIds, + '"Supported runtime names" list must match (capability registry ∪ named legacy inclusions ' + + `[${NAMED_LEGACY_INCLUSIONS.join(', ')}]) minus deliberate exclusions (${NAMED_DELIBERATE_EXCLUSIONS.join(', ')}).\n` + + ` in doc but not expected: ${JSON.stringify(docIds.filter((id) => !expectedIds.includes(id)))}\n` + + ` expected but missing from doc: ${JSON.stringify(expectedIds.filter((id) => !docIds.includes(id)))}` + ); + assert.deepStrictEqual( + toAllIds, expectedIds, + '`--to all` TO_RUNTIMES expansion must match (capability registry ∪ named legacy inclusions ' + + `[${NAMED_LEGACY_INCLUSIONS.join(', ')}]) minus deliberate exclusions (${NAMED_DELIBERATE_EXCLUSIONS.join(', ')}).\n` + + ` in --to-all but not expected: ${JSON.stringify(toAllIds.filter((id) => !expectedIds.includes(id)))}\n` + + ` expected but missing from --to-all: ${JSON.stringify(expectedIds.filter((id) => !toAllIds.includes(id)))}` + ); + }); + test('idempotency documented (second apply = zero changes)', () => { content = content || readWorkflow(); assert.ok( @@ -10259,11 +10588,16 @@ describe('sync-skills.md — required behavioral specs', () => { ); }); - test('install.js --skills-root is used for path resolution', () => { + test('gsd_run query skills-root is used for path resolution (#3024)', () => { + // #3024: sync-skills.md moved off the unshipped `install.js --skills-root` + // entry point onto `gsd_run query skills-root`, which IS shipped (routes + // through gsd-tools.cjs — see routeSkillsRoot). This asserts the new + // contract; the old install.js reference is covered as an explicit + // regression by "defect C: workflow contains zero references to install.js". content = content || readWorkflow(); assert.ok( - content.includes('--skills-root'), - 'workflow must reference install.js --skills-root for path resolution' + content.includes('gsd_run query skills-root'), + 'workflow must reference gsd_run query skills-root for path resolution' ); }); @@ -10290,6 +10624,155 @@ describe('sync-skills.md — required behavioral specs', () => { 'workflow must have a safety rule that dry-run performs no writes' ); }); + + // Defect C regression: the workflow must not send users back to the + // unshipped `install.js` entry point this issue moved away from (#3024). + test('defect C: workflow contains zero references to install.js', () => { + content = content || readWorkflow(); + const occurrences = (content.match(/install\.js/g) || []).length; + assert.strictEqual( + occurrences, 0, + `sync-skills.md must not reference install.js (unshipped in installed trees); found ${occurrences} occurrence(s)` + ); + }); + + test('Step 2 resolves skills roots via gsd_run query skills-root', () => { + content = content || readWorkflow(); + assert.ok( + content.includes('gsd_run query skills-root'), + 'workflow Step 2 must resolve skills roots via `gsd_run query skills-root`' + ); + }); + + // #3024 follow-up hardening: neither `gsd_run query skills-root` call in + // Step 2 checked its exit status or for an empty result, so an unregistered + // runtime id silently produced an empty root that Step 5 fed straight into + // `rm -rf`/`cp -r`. These assert the guard is present in the shipped text. + test('Step 2 guards the SOURCE skills-root resolution (exit status + non-empty)', () => { + content = content || readWorkflow(); + const step2 = content.slice(content.indexOf('## Step 2:'), content.indexOf('## Step 3:')); + assert.ok( + /SRC_SKILLS_ROOT\s*=\s*\$\(gsd_run query skills-root[^)]*\)\s*\n\s*if\s*\[\s*\$\?\s*-ne\s*0\s*\]\s*\|\|\s*\[\s*-z\s*"\$SRC_SKILLS_ROOT"\s*\]/.test(step2), + 'Step 2 must check exit status ($?) and non-empty (-z) immediately after resolving SRC_SKILLS_ROOT' + ); + assert.match( + step2, + /exit 1/, + 'Step 2 must exit non-zero when SRC_SKILLS_ROOT resolution fails' + ); + }); + + test('Step 2 guards each DESTINATION skills-root resolution (exit status + non-empty)', () => { + content = content || readWorkflow(); + const step2 = content.slice(content.indexOf('## Step 2:'), content.indexOf('## Step 3:')); + assert.ok( + /for DEST_RUNTIME in "\$\{TO_RUNTIMES\[@\]\}"; do[\s\S]*?gsd_run query skills-root "\$DEST_RUNTIME"[^)]*\)[\s\S]*?if\s*\[\s*\$\?\s*-ne\s*0\s*\]\s*\|\|\s*\[\s*-z\s*"\$RESOLVED_DEST_ROOT"\s*\][\s\S]*?exit 1[\s\S]*?done/.test(step2), + 'Step 2 must check exit status and non-empty for each resolved destination root inside the TO_RUNTIMES loop, and exit 1 on failure' + ); + }); + + test('prose guard documents destination skills-root resolution failure (mirrors source guard)', () => { + content = content || readWorkflow(); + assert.ok( + /resolving the skills root for the source OR any destination runtime fails/i.test(content), + 'workflow must document a guard for destination skills-root resolution failure alongside the source-not-found guard' + ); + }); + + test('Step 5 requires non-empty/absolute roots before any rm -rf or cp -r', () => { + content = content || readWorkflow(); + const step5 = content.slice(content.indexOf('## Step 5:')); + const rmIndex = step5.indexOf('rm -rf "$DEST_ROOT/$SKILL"'); + const cpIndex = step5.indexOf('cp -r "$SRC_SKILLS_ROOT/$SKILL"'); + assert.ok(rmIndex > -1, 'Step 5 must contain rm -rf "$DEST_ROOT/$SKILL"'); + assert.ok(cpIndex > -1, 'Step 5 must contain cp -r "$SRC_SKILLS_ROOT/$SKILL"'); + + const srcGuardIndex = step5.indexOf('[[ "$SRC_SKILLS_ROOT" == /* ]]'); + const destGuardIndex = step5.indexOf('[[ "$DEST_ROOT" == /* ]]'); + assert.ok(srcGuardIndex > -1, 'Step 5 must guard SRC_SKILLS_ROOT as a non-empty absolute path before use'); + assert.ok(destGuardIndex > -1, 'Step 5 must guard DEST_ROOT as a non-empty absolute path before use'); + assert.ok( + srcGuardIndex < rmIndex && destGuardIndex < rmIndex, + 'the absolute-path guards for SRC_SKILLS_ROOT and DEST_ROOT must precede the first rm -rf in Step 5' + ); + assert.ok( + srcGuardIndex < cpIndex && destGuardIndex < cpIndex, + 'the absolute-path guards for SRC_SKILLS_ROOT and DEST_ROOT must precede cp -r in Step 5' + ); + }); + + // #3024 follow-up (this fix): `DEST_SKILLS_ROOTS` was assigned in Step 2 as a + // keyed map but never `declare -A`'d and never read back anywhere — on bash 3.2 + // (macOS system /bin/bash) that assignment silently collapses every destination's + // resolved root into index [0], and since nothing ever reads it, Steps 3/5's + // `$DEST_ROOT` was always unbound. The fix drops the array entirely; this pins + // the regression so it cannot silently come back. + test('defect: DEST_SKILLS_ROOTS array is gone (bash-3.2 hazard, was never read)', () => { + content = content || readWorkflow(); + assert.ok( + !content.includes('DEST_SKILLS_ROOTS'), + 'workflow must not reference DEST_SKILLS_ROOTS anywhere; it was an unread, ' + + 'bash-3.2-hostile associative-array assignment' + ); + }); + + test('no associative-array syntax anywhere in the file (bash 3.2 compatibility)', () => { + content = content || readWorkflow(); + assert.ok( + !/\bdeclare\s+-A\b/.test(content), + 'workflow must not use `declare -A` (bash4+-only; system /bin/bash on macOS is 3.2)' + ); + assert.ok( + !/\btypeset\s+-A\b/.test(content), + 'workflow must not use `typeset -A` (bash4+-only associative-array declaration)' + ); + }); + + // #3024 follow-up (this fix): every fenced bash block that reads `$DEST_ROOT` + // must itself assign `DEST_ROOT` before that read — Steps 3 and 5 are separate + // bash constructs (not one continuously-executing script), so each one has to + // bind DEST_ROOT for the destination it is currently processing rather than + // relying on a value threaded in from elsewhere. This is the actual fix for the + // dangling-variable defect; assert it holds for every ```bash block in the file, + // not just Steps 3/5, so a future edit that introduces a new $DEST_ROOT read + // elsewhere is held to the same rule. + test('every $DEST_ROOT read is preceded by a DEST_ROOT= assignment in the same bash block', () => { + content = content || readWorkflow(); + const bashBlocks = [...content.matchAll(/```bash\r?\n([\s\S]*?)```/g)].map((m) => m[1]); + assert.ok( + bashBlocks.length > 0, + 'extractor matched no fenced ```bash blocks at all — the workflow must contain some' + ); + + const blocksUsingDestRoot = bashBlocks.filter((block) => /\$DEST_ROOT\b/.test(block)); + assert.ok( + blocksUsingDestRoot.length > 0, + 'extractor found no fenced bash block referencing $DEST_ROOT — expected Step 3 and Step 5 to reference it' + ); + // This is the exact bug: Steps 3 and 5 both use $DEST_ROOT, so there must be at + // least two such blocks (one per step). A single match would mean one of the two + // steps lost its reference to $DEST_ROOT entirely rather than being fixed. + assert.ok( + blocksUsingDestRoot.length >= 2, + 'expected at least 2 fenced bash blocks referencing $DEST_ROOT (Step 3 and Step 5), got ' + + String(blocksUsingDestRoot.length) + ); + + for (const block of blocksUsingDestRoot) { + const assignMatch = block.match(/^\s*DEST_ROOT=/m); + assert.ok( + assignMatch, + 'a fenced bash block reads $DEST_ROOT but never assigns it: ' + JSON.stringify(block.slice(0, 200)) + ); + const assignIndex = block.indexOf(assignMatch[0]); + const firstReadIndex = block.search(/\$DEST_ROOT\b/); + assert.ok( + firstReadIndex > -1 && assignIndex <= firstReadIndex, + 'DEST_ROOT= assignment (index ' + assignIndex + ') must precede the first $DEST_ROOT read ' + + '(index ' + firstReadIndex + ') in the same bash block: ' + JSON.stringify(block.slice(0, 200)) + ); + } + }); }); // ── commands/gsd/sync-skills.md ─────────────────────────────────────────────── diff --git a/tests/runtime-homes-legacy-ids-drift-guard.test.cjs b/tests/runtime-homes-legacy-ids-drift-guard.test.cjs new file mode 100644 index 000000000..2081e20aa --- /dev/null +++ b/tests/runtime-homes-legacy-ids-drift-guard.test.cjs @@ -0,0 +1,194 @@ +'use strict'; + +/** + * #3024 review finding 2 — drift guard for LEGACY_NON_REGISTRY_RUNTIME_IDS. + * + * isRegisteredRuntimeId() accepts an id if it is either a capability-registry + * key or a member of the hand-maintained LEGACY_NON_REGISTRY_RUNTIME_IDS set + * (currently just `grok`). That set is a SECOND hand-maintained proxy for the + * real predicate — "does this id have a genuine runtime-specific resolution + * in getGlobalConfigDir, distinct from the generic claude fallback?" — + * mirroring the exact mistake that caused the grok regression (the registry + * was the first such proxy, and it silently misclassified grok). Nothing + * currently stops a third hardcoded branch being added to getGlobalConfigDir + * without anyone updating the Set. + * + * DESIGN (do not "fix" by making production logic derive the answer at + * runtime): production code stays an explicit, greppable Set. Deriving the + * predicate at runtime by diffing against a sentinel resolution would make + * validation depend on a heuristic comparison against that sentinel, which is + * harder to reason about and could misfire for an id that legitimately + * shares claude's directory. Instead, THIS TEST derives the ground truth from + * the compiled module's actual behavior and fails loudly the moment the + * hand-maintained Set falls out of sync with it, in either direction. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.join(__dirname, '..'); +const LIB_PATH = path.join(ROOT, 'gsd-core', 'bin', 'lib', 'runtime-homes.cjs'); +const CAPABILITY_REGISTRY_PATH = path.join(ROOT, 'gsd-core', 'bin', 'lib', 'capability-registry.cjs'); + +const { getGlobalConfigDir, LEGACY_NON_REGISTRY_RUNTIME_IDS } = require(LIB_PATH); +const { runtimes } = require(CAPABILITY_REGISTRY_PATH); + +// A sentinel id that is definitely unregistered and has no dedicated branch: +// resolving it teaches us what the generic (claude) fallback path is. +const SENTINEL_ID = 'zzz-not-a-runtime-3024-drift-guard'; + +/** + * Every env var a descriptor-driven runtime, or a hardcoded branch, reads to + * override its resolved directory. Derived from the registry itself (not + * hand-copied) plus the one variable consumed by getGlobalConfigDir's grok + * branch, which lives outside the registry entirely. Cleared for the + * duration of each test so ambient env vars in the test-runner's environment + * cannot change a runtime's resolved path out from under the assertions. + */ +function collectDescriptorEnvVars() { + const vars = new Set(['GROK_AGENTS_HOME']); + for (const entry of Object.values(runtimes)) { + const configHome = entry.runtime?.configHome; + if (configHome?.env) configHome.env.forEach((v) => vars.add(v)); + if (configHome?.skillsHome?.env) configHome.skillsHome.env.forEach((v) => vars.add(v)); + } + assert.ok(vars.size > 1, 'EMPTY CAPTURE: derived zero descriptor env vars from the capability registry'); + return vars; +} + +function clearEnv(keys) { + const saved = {}; + for (const k of keys) { + saved[k] = process.env[k]; + delete process.env[k]; + } + return saved; +} + +function restoreEnv(saved) { + for (const [k, v] of Object.entries(saved)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } +} + +/** + * Parse getGlobalConfigDir's compiled source body for literal + * `runtime === ''` hardcoded branches (grok's, and any future one). This + * is enumeration scaffolding only — every id it turns up is then verified + * BEHAVIORALLY below by actually calling getGlobalConfigDir, never trusted + * on its own. + */ +function parseHardcodedBranchIds(libSource) { + const fnMarker = 'function getGlobalConfigDir('; + const fnStart = libSource.indexOf(fnMarker); + assert.notStrictEqual( + fnStart, + -1, + 'EMPTY CAPTURE: could not locate getGlobalConfigDir in the compiled lib source', + ); + const nextFn = libSource.indexOf('\nfunction ', fnStart + fnMarker.length); + const fnBody = nextFn === -1 ? libSource.slice(fnStart) : libSource.slice(fnStart, nextFn); + assert.ok(fnBody.length > 50, 'EMPTY CAPTURE: getGlobalConfigDir body implausibly short'); + + const ids = []; + const branchIdRe = /runtime\s*===\s*'([^']+)'/g; + let m; + while ((m = branchIdRe.exec(fnBody))) ids.push(m[1]); + assert.ok( + ids.length > 0, + 'EMPTY CAPTURE: parsed zero hardcoded-branch ids out of getGlobalConfigDir — the regex or function-body bound is broken', + ); + return ids; +} + +/** Resolve `id`, classifying it runtime-specific if it differs from `fallbackPath`. */ +function resolveCandidate(id, fallbackPath) { + try { + const resolved = getGlobalConfigDir(id); + return { id, resolved, runtimeSpecific: resolved !== fallbackPath }; + } catch (err) { + // configHome.kind === 'none' (e.g. vscode) throws instead of resolving — + // a distinct, deliberate, definitely-not-the-fallback outcome. + return { id, resolved: ``, runtimeSpecific: true }; + } +} + +describe('#3024 review finding 2: LEGACY_NON_REGISTRY_RUNTIME_IDS drift guard', () => { + test('every runtime-specific id is registered or legacy-listed, and every legacy entry still earns its exemption', (t) => { + const registryIds = Object.keys(runtimes); + assert.ok(registryIds.length > 0, 'EMPTY CAPTURE: capability registry produced zero runtime ids'); + + const legacyIds = Array.from(LEGACY_NON_REGISTRY_RUNTIME_IDS); + assert.ok(legacyIds.length > 0, 'EMPTY CAPTURE: LEGACY_NON_REGISTRY_RUNTIME_IDS is empty'); + + const libSource = fs.readFileSync(LIB_PATH, 'utf-8'); + const sourceParsedIds = parseHardcodedBranchIds(libSource); + + assert.ok( + !registryIds.includes(SENTINEL_ID) && + !legacyIds.includes(SENTINEL_ID) && + !sourceParsedIds.includes(SENTINEL_ID), + `sentinel id ${SENTINEL_ID} unexpectedly collides with a real candidate id — pick a different sentinel`, + ); + + const saved = clearEnv(collectDescriptorEnvVars()); + t.after(() => restoreEnv(saved)); + + const fallbackPath = getGlobalConfigDir(SENTINEL_ID); + assert.ok( + typeof fallbackPath === 'string' && fallbackPath.length > 0, + 'sentinel resolution produced no usable fallback path', + ); + + const candidates = Array.from(new Set([...registryIds, ...legacyIds, ...sourceParsedIds])); + const allowed = new Set([...registryIds, ...legacyIds]); + const resolutions = candidates.map((id) => resolveCandidate(id, fallbackPath)); + + const undeclaredSpecific = resolutions + .filter((r) => r.runtimeSpecific && !allowed.has(r.id)) + .map((r) => r.id); + assert.deepStrictEqual( + undeclaredSpecific, + [], + `id(s) resolve runtime-specifically but are in neither the capability registry nor ` + + `LEGACY_NON_REGISTRY_RUNTIME_IDS: ${JSON.stringify(undeclaredSpecific)}. Remedy: add the id(s) to ` + + `LEGACY_NON_REGISTRY_RUNTIME_IDS in src/runtime-homes.cts (only after confirming the branch is real ` + + `and intended).`, + ); + + const staleLegacy = legacyIds.filter( + (id) => !resolutions.find((r) => r.id === id)?.runtimeSpecific, + ); + assert.deepStrictEqual( + staleLegacy, + [], + `LEGACY_NON_REGISTRY_RUNTIME_IDS entry(ies) no longer resolve runtime-specifically: ` + + `${JSON.stringify(staleLegacy)}. Remedy: remove the stale entry(ies) from ` + + `LEGACY_NON_REGISTRY_RUNTIME_IDS in src/runtime-homes.cts.`, + ); + + const grok = resolutions.find((r) => r.id === 'grok'); + assert.ok(grok, 'grok must appear among the resolved candidates'); + assert.strictEqual( + grok.runtimeSpecific, + true, + 'grok must resolve runtime-specifically (its hardcoded branch is the reason LEGACY_NON_REGISTRY_RUNTIME_IDS exists)', + ); + }); + + test('gemini (an unregistered id with no dedicated branch) resolves to the generic fallback, not runtime-specifically', (t) => { + const saved = clearEnv(collectDescriptorEnvVars()); + t.after(() => restoreEnv(saved)); + + const fallbackPath = getGlobalConfigDir(SENTINEL_ID); + assert.strictEqual( + getGlobalConfigDir('gemini'), + fallbackPath, + 'gemini must resolve to the same generic fallback as an unregistered id — it has no registry descriptor ' + + 'and no dedicated branch', + ); + }); +});