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', + ); + }); +});