diff --git a/.changeset/sturdy-cranes-jump.md b/.changeset/sturdy-cranes-jump.md new file mode 100644 index 000000000..629dcee99 --- /dev/null +++ b/.changeset/sturdy-cranes-jump.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2340 +--- +**Installed third-party capability skills now materialize as real slash commands** — a capability could pass every check (`installed: true, surfaced: true, active: true`) and still never exist on disk: the registry layer counted the capability's skill as surfaced, but the file-copy step only ever scanned gsd-core's own bundled commands, so nothing was ever written to the runtime's `skills/` directory and the command was never invocable. Installed capability skills are now staged from where they live, bound to the capability that actually declared and registered them (never inferred from directory listing order), and are subject to the same runtime-targeted body rewrites as first-party skills — first-party skills still win any name collision. diff --git a/bin/install.js b/bin/install.js index 7180a9424..64d5aa3a8 100755 --- a/bin/install.js +++ b/bin/install.js @@ -414,6 +414,32 @@ try { _capabilityRegistry = undefined; } +// #2322 BLOCKER 2: `_capabilityRegistry` above is the FROZEN first-party registry +// (capability-registry.cjs, built at publish time) — it never reflects an +// INSTALLED third-party overlay capability, so a fresh `gsd install` could never +// stage an installed third-party capability's skill regardless of registration, +// even on the DEFAULT `--profile full`. `_installedCapabilityRegistry` composes +// the overlay via capability-loader's `loadRegistry({includeInstalled:true})` — +// the SAME call capability-writer.cts's `capability set --runtime` path already +// uses — so a fresh install and a post-install `capability set` agree on +// third-party skill availability. Used ONLY for skill-profile resolution and +// runtime-artifact-layout staging below; `_capabilityRegistry` (frozen) remains +// the source for gsd-core's OWN runtime/host-behavior descriptors (unaffected — +// those are always first-party). A load failure degrades to the frozen +// `_capabilityRegistry` (no overlay data -> no third-party skills staged; never +// a crash and never a scan-and-guess fallback). +let _installedCapabilityRegistry; +try { + const _capabilityLoader = require(path.join(_gsdLibDir, 'capability-loader.cjs')); + _installedCapabilityRegistry = _capabilityLoader.loadRegistry({ + includeInstalled: true, + cwd: process.cwd(), + gsdHome: process.env['GSD_HOME'], + }); +} catch (_) { + _installedCapabilityRegistry = _capabilityRegistry; +} + // Fail-safe floor for the reference host's #338-privacy-critical behaviors, used // ONLY when the first-party capability registry cannot be loaded (a broken bundle). // Without it, a registry-load failure would make `_hostBehaviors('claude')` return @@ -9944,14 +9970,24 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { const _effectiveInstallMode = _isCoreProfileAlias ? 'minimal' : 'full'; // Load the manifest and compute resolved profile for named profiles. // For --minimal/core: use an empty manifest (core profile has no transitive - // deps) to produce a resolvedProfile with the core skill set. Registry IS - // consulted so tier:core capability skills are included when registered. + // deps) to produce a resolvedProfile with the core skill set. For core/ + // standard profiles, resolveProfile's `registry` arg IS consulted (via + // _capabilitySkillsForMode) so tier:core/tier:standard capability skills are + // unioned in when registered. #2322 correction: for the DEFAULT `full` + // profile, resolveProfile short-circuits to the `{skills:'*'}` sentinel + // BEFORE ever reading `registry` (there is nothing to union — '*' already + // means "everything"), so the registry consultation that matters for `full` + // happens LATER, at staging time (stageSkillsForRuntimeAsSkills's '*' + // fill-in, resolveRuntimeArtifactLayout's `capabilityRegistry` param below) — + // not here. `_installedCapabilityRegistry` (not the frozen `_capabilityRegistry`) + // is passed so an INSTALLED third-party capability (not just a first-party + // one) is honored on every profile, `full` included (#2322 blocker 2). const _commandsDir = path.join(src, 'commands', 'gsd'); const _skillsManifest = _isCoreProfileAlias ? new Map() : loadSkillsManifest(_commandsDir); const _resolvedProfile = resolveProfile({ modes: [_activeProfileName], manifest: _skillsManifest, - registry: _capabilityRegistry, + registry: _installedCapabilityRegistry, }); // Unified staging function: all profiles use stageSkillsForProfile with the // registry-aware _resolvedProfile (ADR-857 phase 4c cutover). @@ -10311,7 +10347,10 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { resolveAttribution: getCommitAttribution, }); } else { - installRuntimeArtifacts(runtime, targetDir, scope, _resolvedProfile, getCommitAttribution); + // #2322: fallback path (adapter unavailable) — thread the composed + // registry too, so this path stages third-party capability skills + // identically to the primary adapter path above. + installRuntimeArtifacts(runtime, targetDir, scope, _resolvedProfile, getCommitAttribution, _installedCapabilityRegistry); } // #1326 — Codex only: remove stale agents/openai.yaml sidecars from managed diff --git a/src/adapter-imperative.cts b/src/adapter-imperative.cts index 9cfd72947..0cc4fa21a 100644 --- a/src/adapter-imperative.cts +++ b/src/adapter-imperative.cts @@ -62,12 +62,20 @@ export function createImperativeAdapter( runtime, registry, install(intent: AdapterInstallIntent): void { + // #2322: thread the SAME composed registry (loaded above, includeInstalled:true) + // this adapter exposes via `.registry` into the engine call, so the skills + // kind's stage() closure can bind an installed third-party capability + // skill to its declaring capId at staging time — this is the PRIMARY + // install path (bin/install.js prefers the adapter over the direct + // installRuntimeArtifacts fallback), so without this the adapter path + // never staged a third-party capability skill regardless of registration. installEngine.installRuntimeArtifacts( runtime, intent.configDir, intent.scope, intent.resolvedProfile, intent.resolveAttribution, + registry, ); }, uninstall(intent: AdapterUninstallIntent): void { diff --git a/src/capability-writer.cts b/src/capability-writer.cts index 07d393309..e9f48c6fe 100644 --- a/src/capability-writer.cts +++ b/src/capability-writer.cts @@ -355,10 +355,15 @@ function setCapabilityState( // eslint-disable-next-line @typescript-eslint/no-require-imports const runtimeArtifactLayout = require('./runtime-artifact-layout.cjs') as { // eslint-disable-next-line @typescript-eslint/no-explicit-any - resolveRuntimeArtifactLayout: (runtime: string, configDir: string, scope: string) => any; + resolveRuntimeArtifactLayout: (runtime: string, configDir: string, scope: string, capabilityRegistry?: unknown) => any; }; + // #2322: thread the SAME composed registry (loaded above, includeInstalled:true) + // into layout resolution so the skills kind's stage() closure can bind a + // third-party capability skill to its declaring capId at staging time — + // required for BOTH the '*' (full-profile) fill-in and the ownership binding + // (see resolveRuntimeArtifactLayout's #2322 doc comment). // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const layout = runtimeArtifactLayout.resolveRuntimeArtifactLayout(runtime, resolvedConfigDir, scope); + const layout = runtimeArtifactLayout.resolveRuntimeArtifactLayout(runtime, resolvedConfigDir, scope, registry); const commandsGsdDir = _resolveCommandsGsdDir(); const manifest = _resolveManifest(commandsGsdDir, resolvedConfigDir); // #1575: applySurface now accepts opts.resolveAttribution so surface-path diff --git a/src/external-descriptor-trust.cts b/src/external-descriptor-trust.cts index 1e755310f..5557fa2de 100644 --- a/src/external-descriptor-trust.cts +++ b/src/external-descriptor-trust.cts @@ -22,9 +22,21 @@ import path from 'node:path'; /** - * Pure path-containment check (cross-platform). `target` is confined to `root` - * iff resolving it relative to `root` yields a path equal to or under `root`. + * Pure LEXICAL path-containment check (cross-platform). `target` is confined + * to `root` iff resolving it relative to `root` (via `path.resolve` — string + * manipulation, no filesystem access) yields a path equal to or under `root`. * Absolute paths outside `root` and `..`-escapes return false. + * + * NOT a realpath check: this function never calls `fs.realpathSync` and does + * not detect a symlink along `target` (or an existing path component of + * `root`) that would redirect the LEXICALLY-confined path to a physically + * different, unconfined location on disk. A caller relying on this for a + * write-confinement guarantee against a symlink-planting attacker must pair + * it with a symlink check (or refuse to follow symlinks at write time) — see + * capability-source.cts's install adapters (:491,577,675), which is what + * currently keeps every caller of this function's callers symlink-safe: they + * reject symlinks upstream, before a target ever reaches a lexical-only check + * like this one. */ export function isPathConfined(target: string, root: string): boolean { if (typeof target !== 'string' || typeof root !== 'string' || target.length === 0 || root.length === 0) { diff --git a/src/install-engine.cts b/src/install-engine.cts index 4910a6938..2404f5f04 100644 --- a/src/install-engine.cts +++ b/src/install-engine.cts @@ -600,6 +600,11 @@ function _runLegacyUninstallCleanup(runtime: string, configDir: string, scope: s * @param scope * @param resolvedProfile from resolveProfile() / resolveEffectiveProfile() * @param resolveAttribution injection: (runtime) => attribution string | undefined + * @param capabilityRegistry #2322: optional composed capability registry + * (capabilityClusters view) — threaded into resolveRuntimeArtifactLayout so + * the skills kind can materialize installed third-party capability skills + * bound to their declaring capId. Absent -> no third-party skills staged + * (fail closed), matching the layout resolver's own optional-registry contract. */ function installRuntimeArtifacts( runtime: string, @@ -607,6 +612,7 @@ function installRuntimeArtifacts( scope: string, resolvedProfile: any, resolveAttribution: ResolveAttribution = () => undefined, + capabilityRegistry?: any, ): void { // Combined-family runtimes (OpenCode/Kilo, ADR-1239 / #2087): route through // the dedicated combined commands+skills+plugin orchestrator instead of the @@ -621,7 +627,7 @@ function installRuntimeArtifacts( // Legacy cleanup before layout-driven writes _runLegacyInstallMigrations(runtime, configDir, scope); - const layout = runtimeArtifactLayout.resolveRuntimeArtifactLayout(runtime, configDir, scope as 'global' | 'local'); + const layout = runtimeArtifactLayout.resolveRuntimeArtifactLayout(runtime, configDir, scope as 'global' | 'local', capabilityRegistry); const planResult = runtimeArtifactInstallPlan.createRuntimeArtifactInstallPlan({ // `Layout` is structurally identical across the layout/install-plan .cjs // modules but nominally distinct to tsc (untyped .cjs boundary) — bridge it. diff --git a/src/install-profiles.cts b/src/install-profiles.cts index fd6bfba76..96c18fced 100644 --- a/src/install-profiles.cts +++ b/src/install-profiles.cts @@ -11,6 +11,9 @@ import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; import { platformWriteSync } from './shell-command-projection.cjs'; +// #2322: reuse the existing pure path-containment seam (ADR-1239 Phase C-2) +// instead of hand-rolling a new traversal check for capability skill stems. +import { isPathConfined } from './external-descriptor-trust.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import conversionModule = require('./runtime-artifact-conversion.cjs'); const { @@ -470,12 +473,174 @@ function transformRouterBodyToNested(converted: string): string { return out.join('\n'); } +/** + * #2322 SECURITY: a third-party `capability.json`'s `skills[]` entries are only + * validated for being STRINGS and not one of the 3 reserved prototype-pollution + * names (capability-validator.cjs validateFeatureBody, ~line 503) — NOT for + * non-emptiness and NOT for a safe path-segment shape. `isSafeCapabilitySkillStem` + * is therefore the SOLE defense against an empty-string, `..`-escaping, + * separator-carrying, absolute, or NUL-carrying stem reaching a filesystem path + * as a literal component — not a second defense-in-depth layer on top of any + * validator-enforced non-emptiness (there is none). Once unioned into + * resolveSurface's `resolved.skills` (#2045), such a stem must never reach + * fs.readFileSync/writeFileSync as a literal path component, or it can escape + * the capabilities root on read (or stageDir on write). Reject anything but a + * single, ordinary path segment. + */ +function isSafeCapabilitySkillStem(stem: string): boolean { + if (typeof stem !== 'string' || stem.length === 0) return false; + if (stem.includes('\0')) return false; + if (stem === '.' || stem === '..') return false; + if (stem.includes('/') || stem.includes('\\')) return false; + if (path.isAbsolute(stem)) return false; + return true; +} + +/** + * Resolve which capability id DECLARES ownership of `stem`, per the registry's + * `capabilityClusters` view (capId -> [owned skill stems]) — the SAME + * authoritative binding `_capabilitySkillsForMode` (above) and `resolveSurface` + * (surface.cts) already trust to decide which stems a capability contributes. + * `capabilityClusters` is derived (gen-capability-registry.cjs + * deriveCapabilityClusters) straight from each ACCEPTED capability's OWN + * declared, non-empty `skills[]` array — an UNDECLARED directory a capability + * happens to ship on disk (an unlisted `skills//` bundled by mistake, or + * by a malicious author trying to hijack another capability's stem) never + * appears here, so it can never resolve as an owner. Two capabilities can never + * both own the same stem: the registry loader (capability-loader.cts) rejects a + * candidate whose declared skill collides with an already-registered owner + * BEFORE it is ever composed into the registry — so this lookup is unambiguous + * by construction. Returns null for an unowned/unregistered stem or a + * malformed registry (never throws). + */ +function _owningCapabilityId(stem: string, clusters: Record): string | null { + const BANNED = ['__proto__', 'constructor', 'prototype']; + for (const capId of Object.keys(clusters)) { + if (BANNED.includes(capId)) continue; + const owned = clusters[capId]; + if (!Array.isArray(owned)) continue; + if (owned.includes(stem)) return capId; + } + return null; +} + +/** + * Union every stem ANY accepted capability declares across the WHOLE registry + * (unfiltered by mode/tier) — used only for the `'*'` (full profile) staging + * fill-in below, mirroring the SAME unconditional union `resolveSurface` + * (surface.cts) already performs when ITS OWN base profile resolves to `'*'`. + * Guards against a malformed/prototype-polluted registry; never throws. + */ +function capabilityClusterStems(registry: CapabilityRegistry | undefined): Set { + const result = new Set(); + const clusters = registry?.capabilityClusters; + if (!clusters || typeof clusters !== 'object') return result; + const BANNED = ['__proto__', 'constructor', 'prototype']; + for (const capId of Object.keys(clusters)) { + if (BANNED.includes(capId)) continue; + const stems = clusters[capId]; + if (!Array.isArray(stems)) continue; + for (const s of stems) { + if (typeof s === 'string' && s.length > 0) result.add(s); + } + } + return result; +} + +/** + * #2322 HIGH-3: filesystem marker written into every staged THIRD-PARTY + * capability skill directory (alongside SKILL.md) so a later prune pass + * (surface.cts pruneSkillDirs) can identify the directory as GSD-capability- + * owned even after the owning capability has been uninstalled/unsurfaced and + * no longer appears in ANY registry view. Without a persisted marker, an + * orphaned capability skill directory has no first-party manifest entry (the + * skill manifest only ever knows gsd-core's own bundled stems) and + * pruneSkillDirs' conservative unknown-directory branch would preserve it + * FOREVER — uninstalling a malicious capability would never actually remove + * its already-staged instructions from the agent's context. A directory + * WITHOUT this marker is presumed genuinely user-created (data-loss + * protection is unchanged for that case). + */ +const CAPABILITY_SKILL_MARKER = '.gsd-capability-skill'; + +/** + * Look up an installed third-party capability's already-authored SKILL.md for + * `stem`, bound to its DECLARING capability via the registry's + * `capabilityClusters` view (capId -> owned stems) — NEVER by scanning every + * installed capability directory and taking the first (sorted) match. + * + * #2322 BLOCKER 1: the prior implementation scanned every directory under the + * capabilities root for a `skills//SKILL.md` file and returned the FIRST + * SORTED match, regardless of whether that capability actually DECLARED the + * stem in its `capability.json` `skills[]` and regardless of whether it was + * the (sole) REGISTERED owner. An attacker-controlled capability could ship an + * UNDECLARED `skills//SKILL.md` directory that sorted ahead of + * the legitimate, declaring capability and hijack its stem — the agent would + * load the attacker's instructions believing they came from the legitimate + * capability. Resolving `stem -> capId` via `capabilityClusters` FIRST (the + * same authoritative binding `resolveSurface`/`_capabilitySkillsForMode` + * trust) then reading ONLY that capability's own directory makes an + * undeclared/unregistered sibling directory unreachable by construction. + * + * The install-root path convention (`//skills// + * SKILL.md` under `GSD_HOME || homedir()`) mirrors capability-loader.cts + * (global overlay root) and capability-source.cts's `stageValidated` finalDir. + * + * Total/non-throwing (#2322 requirement 5): no registry, an unowned stem, a + * missing capabilities root, an unreadable capability dir, or a missing/ + * corrupt SKILL.md all degrade to `null` (skip that stem) rather than + * throwing — a partial/corrupt third-party install must never break + * first-party staging. No registry at all means NOTHING third-party is + * staged (fail closed — never a fallback scan). + * + * NOTE: the content returned here is staged AS-IS (no per-file `converter` + * runs on it — unlike gsd-core's flat command `.md`, an installed capability + * skill is already a complete SKILL.md), but it is NOT immune from the LATER + * runtime-targeted body rewrite pass `applySurface` runs over the ENTIRE + * staged directory (`rewriteStagedSkillBodies`, surface.cts): a `~/.claude/` + * (etc.) path reference in a third-party skill body IS rewritten exactly like + * a first-party one. "As-is" here refers only to this copy step, not to the + * final on-disk content after a full `applySurface` run. + */ +function readInstalledCapabilitySkill(stem: string, registry: CapabilityRegistry | undefined): { capId: string; content: string } | null { + if (!isSafeCapabilitySkillStem(stem)) return null; + if (!registry || !registry.capabilityClusters || typeof registry.capabilityClusters !== 'object') return null; + const capId = _owningCapabilityId(stem, registry.capabilityClusters); + if (capId === null) return null; + // Defense-in-depth: capId is a real accepted-capability directory name (a + // trusted fs.readdirSync entry at capability-loader.cts accept time), but + // re-validate its path-segment shape before using it as a literal path + // component in case a future registry composer ever stops guaranteeing that. + if (!isSafeCapabilitySkillStem(capId)) return null; + const home = process.env['GSD_HOME'] || os.homedir(); + const capDir = path.join(home, '.gsd', 'capabilities', capId); + const relSkillPath = path.join('skills', stem, 'SKILL.md'); + // Defense-in-depth: isSafeCapabilitySkillStem already rejects separators/ + // '..'/absolute stems, but re-confirm the resolved read path stays under + // this capability's own directory before ever touching the filesystem. + if (!isPathConfined(relSkillPath, capDir)) return null; + const skillPath = path.join(capDir, relSkillPath); + try { + if (!fs.statSync(skillPath).isFile()) return null; + return { capId, content: fs.readFileSync(skillPath, 'utf8') }; + } catch { + return null; // missing / unreadable / corrupt entry -> skip + } +} + +/** + * @param registry optional capability registry (capabilityClusters view) — + * when present, third-party capability skills are unioned into the staged + * output (bound to their declaring capId; see readInstalledCapabilitySkill). + * When absent, NOTHING third-party is staged (fail closed). + */ function stageSkillsForRuntimeAsSkills( srcCommandsDir: string, resolvedProfile: ResolvedProfile, converter: (content: string, skillName: string) => string, prefix: string, nested = false, + registry?: CapabilityRegistry, ): string { if (!fs.existsSync(srcCommandsDir)) return srcCommandsDir; @@ -496,6 +661,11 @@ function stageSkillsForRuntimeAsSkills( } } + // #2322: stems actually staged from gsd-core's OWN bundled commands/gsd dir + // this call, so the third-party fill-in pass below can enforce "first-party + // ALWAYS wins on collision" without re-deriving membership. + const firstPartyStems = new Set(); + const stageDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-profile-runtime-skills-')); try { const entries = fs.readdirSync(srcCommandsDir, { withFileTypes: true }); @@ -504,6 +674,7 @@ function stageSkillsForRuntimeAsSkills( if (!entry.name.endsWith('.md')) continue; const stem = entry.name.slice(0, -3); if (resolvedProfile.skills !== '*' && !(resolvedProfile.skills).has(stem)) continue; + firstPartyStems.add(stem); const content = fs.readFileSync(path.join(srcCommandsDir, entry.name), 'utf8'); const skillName = `${prefix}${stem}`; const converted = converter(content, skillName); @@ -535,6 +706,54 @@ function stageSkillsForRuntimeAsSkills( fs.mkdirSync(destDir, { recursive: true }); fs.writeFileSync(path.join(destDir, 'SKILL.md'), converted); } + + // #2322: materialize installed THIRD-PARTY capability skills, bound to + // their DECLARING capability via the registry's capabilityClusters view + // (see readInstalledCapabilitySkill — NEVER scan-and-first-match). The + // registry union (#2045) already puts every accepted-capability stem into + // a concrete resolvedProfile.skills Set, but srcCommandsDir only ever + // holds gsd-core's own bundled commands — so any stem with no first-party + // file here was silently dropped (registry says surfaced:true, nothing on + // disk) unless we fill it in from the capability's own install dir. + // + // BLOCKER 2 (#2322): `resolveProfile` short-circuits the `full` profile + // straight to the `'*'` sentinel BEFORE ever consulting a registry — the + // sentinel therefore carries no per-stem list of its own, and a bare + // `resolvedProfile.skills !== '*'` gate here skipped this ENTIRE fill-in + // pass for a `full` install regardless of what the registry declared + // (the issue's default-profile repro: `mode=full` staged zero third-party + // skills even when `mode=standard` on the SAME registry staged them + // correctly). When `resolvedProfile.skills === '*'`, the candidate stems + // are instead every stem the registry's `capabilityClusters` declares — + // mirroring the SAME unconditional union `resolveSurface` (surface.cts, + // "Issue #2045" block) already performs for its own `'*'` case. When + // `resolvedProfile.skills` is a concrete Set, the candidate stems are the + // ones `_capabilitySkillsForMode` already unioned into it (unchanged). + // + // No registry in scope at all -> stage NOTHING third-party (fail closed — + // never fall back to scanning). Nesting (#69) never applies to a + // capability skill — it was never a child of any ns-* router's + // `requires:` list — so it always lands flat at the top level, exactly + // like an unrouted first-party skill. + if (registry) { + const candidateStems: Iterable = + resolvedProfile.skills === '*' ? capabilityClusterStems(registry) : resolvedProfile.skills; + for (const stem of candidateStems) { + if (firstPartyStems.has(stem)) continue; // first-party always wins + const found = readInstalledCapabilitySkill(stem, registry); + if (found === null) continue; // absent/malformed/unowned -> skip gracefully + const skillName = `${prefix}${stem}`; + if (!isPathConfined(skillName, stageDir)) continue; // defense-in-depth + const destDir = path.join(stageDir, skillName); + fs.mkdirSync(destDir, { recursive: true }); + fs.writeFileSync(path.join(destDir, 'SKILL.md'), found.content); + // #2322 HIGH-3: persist the capability-owned marker so a later prune + // pass (surface.cts pruneSkillDirs) can identify — and remove — this + // directory even once the owning capability is uninstalled/unsurfaced + // and no longer appears in any registry view. + fs.writeFileSync(path.join(destDir, CAPABILITY_SKILL_MARKER), found.capId + '\n', 'utf8'); + } + } } catch (err) { try { fs.rmSync(stageDir, { recursive: true, force: true }); } catch { /* best-effort */ } throw err; @@ -878,6 +1097,12 @@ export = { parseRequires, parseCallsAgents, cleanupStagedSkills, + // #2322: capability-skill security seams — exported for direct unit-testing + // and for surface.cts's prune pass (CAPABILITY_SKILL_MARKER parity). + isSafeCapabilitySkillStem, + readInstalledCapabilitySkill, + capabilityClusterStems, + CAPABILITY_SKILL_MARKER, // Back-compat / deprecated MINIMAL_SKILL_ALLOWLIST, isMinimalMode, diff --git a/src/runtime-artifact-layout.cts b/src/runtime-artifact-layout.cts index 28339f19c..4a5f8bd8d 100644 --- a/src/runtime-artifact-layout.cts +++ b/src/runtime-artifact-layout.cts @@ -55,6 +55,20 @@ interface ResolvedProfile { agents: Set; } +/** + * #2322: mirrors the (unexported) CapabilityRegistry shape in install-profiles.cts. + * Threaded through resolveRuntimeArtifactLayout -> skillsKind so the skills-kind + * stage() closure can bind a third-party capability skill stem to its DECLARING + * capability (capabilityClusters) at staging time — never by scanning the + * installed capabilities root and guessing. Optional: a caller with no registry + * in scope gets a layout whose skills kind stages NOTHING third-party (fail + * closed), matching install-profiles.cts's own registry-optional contract. + */ +interface CapabilityRegistryForSkills { + capabilityClusters?: Record; + profileMembership?: Record; +} + /** * Cross-cutting context for descriptor-driven agent staging (ADR-1235 §1). * Passed as the optional second arg to ArtifactKind.stage() for agents kind @@ -316,6 +330,10 @@ function kimiAgentsKind(destSubpath: string, prefix: string, configDir: string): * arg so scope-aware converters (antigravity, copilot) can choose * between global home paths and workspace-relative paths without * colliding with the `runtime` string at position 3. + * @param capabilityRegistry #2322: optional capability registry — captured in the + * stage() closure so third-party capability skills are bound to + * their declaring capId at staging time. Absent -> stage() stages + * nothing third-party (fail closed). */ function skillsKind( destSubpath: string, @@ -325,6 +343,7 @@ function skillsKind( configDir: string, nested = false, scope: 'local' | 'global' = 'global', + capabilityRegistry?: CapabilityRegistryForSkills, ): ArtifactKind { return { kind: 'skills', @@ -345,7 +364,7 @@ function skillsKind( const isGlobal = scope === 'global'; const wrappedConverter = (content: string, skillName: string): string => realConverter(content, skillName, runtime, cmdNames, isGlobal); - return stageSkillsForRuntimeAsSkills(findInstallSourceRoot(configDir), resolved, wrappedConverter, prefix, nested); + return stageSkillsForRuntimeAsSkills(findInstallSourceRoot(configDir), resolved, wrappedConverter, prefix, nested, capabilityRegistry); }, }; } @@ -465,7 +484,7 @@ function getRegistry(): RegistryLike { * Map a single ArtifactKindDescriptor entry to an ArtifactKind using the * matching builder function. Mirrors the hand-built calls in the old switch. */ -function dispatchKindEntry(entry: ArtifactKindDescriptor, runtime: string, configDir: string, scope: 'local' | 'global'): ArtifactKind { +function dispatchKindEntry(entry: ArtifactKindDescriptor, runtime: string, configDir: string, scope: 'local' | 'global', capabilityRegistry?: CapabilityRegistryForSkills): ArtifactKind { const { kind, destSubpath, prefix, nesting, converter } = entry; const nested = nesting === 'nested'; @@ -489,7 +508,7 @@ function dispatchKindEntry(entry: ArtifactKindDescriptor, runtime: string, confi `resolveRuntimeArtifactLayout: skills entry for '${runtime}' has converter=null (converter is required for skills)`, ); } - result = skillsKind(destSubpath, prefix, converter, runtime, configDir, nested, scope); + result = skillsKind(destSubpath, prefix, converter, runtime, configDir, nested, scope, capabilityRegistry); break; case 'kimi-agents': @@ -514,9 +533,19 @@ function dispatchKindEntry(entry: ArtifactKindDescriptor, runtime: string, confi * * ADR-857 phase 5d: driven by the capability-registry artifactLayout descriptor * instead of a hardcoded switch statement. + * + * @param capabilityRegistry #2322: optional — when the caller has a composed + * capability registry in scope (e.g. capability-writer.cts's `capability set` + * path, or a fresh install's registry-aware profile resolution), pass it here + * so the skills kind's stage() closure can materialize installed third-party + * capability skills bound to their declaring capId. Both call paths (surface + * apply AND the installer) must pass their registry here — resolveProfile's + * own `'*'` (full profile) short-circuit never carries a registry, so if it + * is not threaded in at layout-build time a `full`-profile install stages no + * third-party capability skills regardless of registration (#2322 blocker 2). */ -function resolveRuntimeArtifactLayout(runtime: string, configDir: string, scope: 'local' | 'global' = 'global'): Layout { - return resolveRuntimeArtifactLayoutFromRegistry(getRegistry(), runtime, configDir, scope); +function resolveRuntimeArtifactLayout(runtime: string, configDir: string, scope: 'local' | 'global' = 'global', capabilityRegistry?: CapabilityRegistryForSkills): Layout { + return resolveRuntimeArtifactLayoutFromRegistry(getRegistry(), runtime, configDir, scope, capabilityRegistry); } function resolveRuntimeArtifactLayoutFromRegistry( @@ -524,6 +553,7 @@ function resolveRuntimeArtifactLayoutFromRegistry( runtime: string, configDir: string, scope: 'local' | 'global' = 'global', + capabilityRegistry?: CapabilityRegistryForSkills, ): Layout { if (typeof configDir !== 'string' || configDir === '') { throw new TypeError('configDir must be a non-empty string'); @@ -538,7 +568,7 @@ function resolveRuntimeArtifactLayoutFromRegistry( } const entries: ArtifactKindDescriptor[] = desc[scope] ?? []; - const kinds: ArtifactKind[] = entries.map((entry) => dispatchKindEntry(entry, runtime, configDir, scope)); + const kinds: ArtifactKind[] = entries.map((entry) => dispatchKindEntry(entry, runtime, configDir, scope, capabilityRegistry)); return { runtime, configDir, scope, kinds }; } diff --git a/src/surface.cts b/src/surface.cts index 436c123ec..1c67adc2b 100644 --- a/src/surface.cts +++ b/src/surface.cts @@ -38,6 +38,10 @@ const { readActiveProfile, resolveProfile, loadSkillsManifest, + // #2322 HIGH-3: shared marker name — single source of truth with the writer + // (install-profiles.cts stageSkillsForRuntimeAsSkills) so the prune reader + // below can never drift from what the stage-time writer actually wrote. + CAPABILITY_SKILL_MARKER, } = installProfiles; import { CLUSTERS } from './clusters.cjs'; import type { ClusterMap } from './clusters.cjs'; @@ -426,12 +430,18 @@ function applySurface(runtimeConfigDir: string, layout: Layout, manifest: Map, prefix: s // Does not match prefix at all — user-owned, preserve. continue; } - if (!canonicalStems) { - // No manifest available: cannot confirm ownership — preserve conservatively. - continue; - } + // #2322: an entry in THIS apply's retained set is unambiguously wanted — + // check that BEFORE the first-party-manifest-membership gate below. The + // manifest only ever knows gsd-core's own bundled stems; a materialized + // third-party capability skill (retained via the resolved profile's + // registry union, #2045/#2322) has no manifest entry at all, so without + // this early check it fell into the "unknown, preserve with warning" + // branch on EVERY apply — misreporting a live, GSD-managed capability + // skill as "user-owned or unknown" noise. This does not change any + // deletion outcome (a retained entry was always preserved — see the + // `retainedNames.has(entry)` check further below); it only short- + // circuits the ambiguous-ownership warning for entries we already know, + // this apply, are wanted. + if (retainedNames.has(entry)) continue; // Finding 1 fix: prefix match is necessary but NOT sufficient. // The dir must also be in the manifest to be considered GSD-owned. // A user-created gsd-* dir that isn't in the manifest is preserved with a warning. - if (!canonicalStems.has(entry.slice(prefix.length))) { + const stem = entry.slice(prefix.length); + if (canonicalStems && canonicalStems.has(stem)) { + isGsdOwned = true; + } else if (fs.existsSync(path.join(entryPath, CAPABILITY_SKILL_MARKER))) { + // #2322 HIGH-3: not a first-party stem, but self-certified as a + // GSD-managed THIRD-PARTY capability skill via the persisted marker + // (written by install-profiles.cts stageSkillsForRuntimeAsSkills at + // stage time). Without this, an orphaned capability skill — its + // owning capability uninstalled/unsurfaced and no longer appearing in + // ANY registry view — had no manifest entry at all and fell into the + // "unknown, preserve with warning" branch below FOREVER: uninstalling + // a malicious capability never actually removed its already-staged + // instructions from the agent's context. The marker makes ownership + // self-certifying at prune time, independent of current registry + // state (or even of whether a manifest was supplied at all). + isGsdOwned = true; + } else if (!canonicalStems) { + // No manifest available and no capability marker: cannot confirm + // ownership — preserve conservatively (silent). + continue; + } else { process.stderr.write( `[gsd] Warning: ${entry} matches GSD prefix '${prefix}' but is not in the manifest — preserving (user-owned or unknown)\n` ); continue; } - isGsdOwned = true; } else if (canonicalStems) { // Hermes: GSD-owned iff the directory name appears in the canonical manifest. isGsdOwned = canonicalStems.has(entry); diff --git a/tests/adapter-imperative.test.cjs b/tests/adapter-imperative.test.cjs index ca5f0fce7..c3ddb00d1 100644 --- a/tests/adapter-imperative.test.cjs +++ b/tests/adapter-imperative.test.cjs @@ -81,7 +81,21 @@ test('imperative adapter.install/uninstall delegate in-process to install-engine installEngine.uninstallRuntimeArtifacts = function (...a) { uninstallArgs = a; return undefined; }; adapter.install({ configDir: '/tmp/imp/' + r, scope: 'global', resolvedProfile: { p: 1 } }); adapter.uninstall({ configDir: '/tmp/imp/' + r, scope: 'local' }); - assert.deepStrictEqual(installArgs, [r, '/tmp/imp/' + r, 'global', { p: 1 }, undefined], `${r}: install delegation args`); + // The trailing arg is the composed capability registry (#2322): the adapter + // must forward one, or third-party capability skills never materialize on + // the default `full` install path. Its contents are the loader's business — + // pin only that a registry-shaped value is threaded through, not its bulk. + const [, , , , manifest, registry] = installArgs; + assert.deepStrictEqual( + installArgs.slice(0, 5), + [r, '/tmp/imp/' + r, 'global', { p: 1 }, undefined], + `${r}: install delegation args`, + ); + assert.strictEqual(manifest, undefined, `${r}: manifest arg unchanged`); + assert.ok( + registry && typeof registry === 'object' && 'capabilityClusters' in registry, + `${r}: install must forward a composed capability registry (#2322), got: ${typeof registry}`, + ); assert.deepStrictEqual(uninstallArgs, [r, '/tmp/imp/' + r, 'local'], `${r}: uninstall delegation args`); } } finally { diff --git a/tests/capability-cli.test.cjs b/tests/capability-cli.test.cjs index 909ef4f77..111aff81e 100644 --- a/tests/capability-cli.test.cjs +++ b/tests/capability-cli.test.cjs @@ -1084,3 +1084,89 @@ describe('capability consent store (#1459)', () => { assert.match(`${r.error}\n${r.output}`, /trust/i); }); }); + +// ─── issue #2322: capability set --runtime materializes installed skills ────── + +describe('issue-2322: capability set --runtime materializes an installed third-party capability skill end-to-end', () => { + /** + * A conformant local capability source that declares one owned skill + * (`skills: [stem]`) and ships that skill's already-authored SKILL.md + * verbatim under `skills//SKILL.md` — mirroring the real on-disk + * shape `capability install` copies into `~/.gsd/capabilities//`. + */ + function writeCapSourceWithSkill(id, stem, skillContent, { tier = 'standard' } = {}) { + const src = tmpDir(`cap-cli-src-${id}-`); + const cap = { + id, + role: 'feature', + version: '1.0.0', + title: id, + description: 'test capability with a skill', + tier, + requires: [], + runtimeCompat: { supported: ['*'], unsupported: [] }, + skills: [stem], + agents: [], + hooks: [], + config: {}, + steps: [], + contributions: [], + gates: [], + }; + fs.writeFileSync(path.join(src, 'capability.json'), JSON.stringify(cap, null, 2)); + const skillDir = path.join(src, 'skills', stem); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), skillContent, 'utf8'); + return src; + } + + test('reproduces #2322: capability state reports surfaced:true but the skill is not written to /skills/', () => { + const home = tmpDir('cap-cli-home-'); + const cwd = makeCwd(); + // #2322 MEDIUM-4: a fixture with NO rewrite-triggering content would pass + // the byte-for-byte assertion below vacuously (it never proves the + // runtime body rewrite pass — which DOES run over third-party skills too — + // actually ran). Embed a `~/.claude/` reference so the later assertion + // exercises that pass instead of asserting its absence. + const AUTHORED = '---\nname: my-thing\ndescription: third-party test skill\n---\n\n# My Thing\nSee @~/.claude/workflows/example.md for details.\n'; + const src = writeCapSourceWithSkill('my-thing', 'my-thing', AUTHORED); + + // Step 2 of the issue repro: capability install ./capabilities/my-thing --yes + const install = runGsdTools(['capability', 'install', src, '--scope', 'global', '--raw'], cwd, scopeEnv(home)); + assert.equal(install.success, true, `install failed: ${install.error || install.output}`); + assert.equal(parse(install.output).status, 'installed'); + assert.ok(fs.existsSync(path.join(capDir(home, 'my-thing'), 'skills', 'my-thing', 'SKILL.md')), 'sanity: install copies the skill verbatim into the capabilities root'); + + // Step 3 of the issue repro: capability set my-thing --runtime claude --scope global + const rcd = tmpDir('cap-cli-rcd-'); + const setResult = runGsdTools( + ['capability', 'set', 'my-thing', '--runtime', 'claude', '--scope', 'global', '--config-dir', rcd, '--raw'], + cwd, + scopeEnv(home), + ); + assert.equal(setResult.success, true, `capability set failed: ${setResult.error || setResult.output}`); + const setOut = parse(setResult.output); + assert.deepEqual(setOut.errors, [], `capability set reported errors: ${JSON.stringify(setOut.errors)}`); + + // Step 4 of the issue repro: capability state --raw -> installed:true, surfaced:true + const myThing = setOut.capabilities.find((c) => c.id === 'my-thing'); + assert.ok(myThing, 'my-thing present in the returned capability state'); + assert.equal(myThing.installed, true, 'installed:true (matches issue step 4)'); + assert.equal(myThing.surfaced, true, 'surfaced:true (matches issue step 4 — #2045 registry union already works)'); + + // Step 5 of the issue repro: ls ~/.claude/skills/gsd-my-thing/ -> No such file or directory (THE BUG) + const stagedPath = path.join(rcd, 'skills', 'gsd-my-thing', 'SKILL.md'); + assert.ok( + fs.existsSync(stagedPath), + `gsd-my-thing/SKILL.md must exist under ${rcd}/skills/ — capability state reports surfaced:true but nothing was materialized to disk (#2322)`, + ); + const staged = fs.readFileSync(stagedPath, 'utf8'); + assert.notEqual( + staged, + AUTHORED, + '#2322 MEDIUM-4: a third-party skill is NOT immune from the runtime-targeted body rewrite pass that runs over the whole stage dir (rewriteStagedSkillBodies) — a byte-for-byte-equal assertion here would be vacuous', + ); + assert.ok(!staged.includes('~/.claude/'), 'the ~/.claude/ literal must have been rewritten to the resolved path prefix'); + assert.ok(staged.includes('workflows/example.md'), 'the rewrite must preserve the referenced path suffix, not corrupt the body'); + }); +}); diff --git a/tests/runtime-artifact-layout-install-profiles.test.cjs b/tests/runtime-artifact-layout-install-profiles.test.cjs index 6361ec939..dfe8d2031 100644 --- a/tests/runtime-artifact-layout-install-profiles.test.cjs +++ b/tests/runtime-artifact-layout-install-profiles.test.cjs @@ -32,6 +32,11 @@ const { writeActiveProfile, PROFILES, STAGED_DIRS, + // #2322 security-hardening seams + isSafeCapabilitySkillStem, + readInstalledCapabilitySkill, + capabilityClusterStems, + CAPABILITY_SKILL_MARKER, } = require('../gsd-core/bin/lib/install-profiles.cjs'); const { createTempDir, cleanup } = require('./helpers.cjs'); @@ -936,6 +941,534 @@ describe('bug-3659: applySurface prunes ~/.claude/skills/gsd-*/ on cluster disab } +// ──────────────────────────────────────────────────────────────────────── +// #2322: applySurface never materializes installed third-party capability +// skills (~/.gsd/capabilities//skills//SKILL.md) — the registry +// (#2045) reports surfaced:true but stageSkillsForRuntimeAsSkills only ever +// iterates findInstallSourceRoot(configDir) (gsd-core's own bundled +// commands/gsd), so a stem that lives only under the capabilities root is +// silently dropped: no error, no file on disk, /gsd- not invocable. +// ──────────────────────────────────────────────────────────────────────── +{ + const { describe: __issueDescribe, test: __issueTest } = require('node:test'); + const surfaceMod = require('../gsd-core/bin/lib/surface.cjs'); + const { writeSurface: __writeSurface, applySurface: __applySurface } = surfaceMod; + const { resolveRuntimeArtifactLayout: __resolveRuntimeArtifactLayout } = require('../gsd-core/bin/lib/runtime-artifact-layout.cjs'); + + /** Install a fake already-installed third-party capability skill under a sandboxed GSD_HOME. */ + function __installCapSkill(gsdHome, capId, stem, content) { + const skillDir = path.join(gsdHome, '.gsd', 'capabilities', capId, 'skills', stem); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), content, 'utf8'); + } + + /** A minimal capability-registry shape carrying one capId -> [stems] cluster. */ + function __registryFor(capId, stems, tier = 'standard') { + return { + capabilityClusters: { [capId]: stems }, + profileMembership: { [capId]: { tier, profiles: tier === 'core' ? ['core', 'standard', 'full'] : ['standard', 'full'] } }, + }; + } + + /** + * Recursively snapshot every FILE under dir into a Map. + * NOTE: converters embed the absolute configDir in some skill bodies (e.g. + * `@`-workflow-file references), so two DIFFERENT tmp configDirs never + * produce byte-identical output even with identical inputs. Every + * before/after comparison below therefore re-applies to the SAME configDir + * (snapshot -> mutate -> re-snapshot) rather than comparing two distinct + * directories. + */ + function __snapshotDir(dir) { + const snap = new Map(); + const walk = (rel) => { + const abs = path.join(dir, rel); + for (const entry of fs.readdirSync(abs, { withFileTypes: true })) { + const relChild = path.join(rel, entry.name); + if (entry.isDirectory()) walk(relChild); + else if (entry.isFile()) snap.set(relChild, fs.readFileSync(path.join(dir, relChild), 'utf8')); + } + }; + walk('.'); + return snap; + } + + /** Assert every entry captured in `before` (a __snapshotDir Map) still exists, byte-identical, under dir. */ + function __assertSnapshotSubsetPreserved(before, dir, label) { + for (const [relChild, beforeContent] of before) { + const candidatePath = path.join(dir, relChild); + assert.ok(fs.existsSync(candidatePath), `${label}: ${relChild} must still exist`); + assert.strictEqual(fs.readFileSync(candidatePath, 'utf8'), beforeContent, `${label}: ${relChild} must be byte-identical`); + } + } + + __issueDescribe('issue-2322: applySurface materializes installed third-party capability skills', () => { + __issueTest('(1) primary: installed capability skill materializes at /SKILL.md and IS subject to the same runtime body rewrites as first-party content', () => { + const gsdHome = createTempDir('gsd-2322-home-'); + const configDir = createTempDir('gsd-2322-cfg-'); + const savedHome = process.env.GSD_HOME; + try { + // #2322 MEDIUM-4: a fixture with NO rewrite-triggering content passes + // vacuously (fs.readFileSync === AUTHORED is trivially true whether or + // not the rewrite pass ever ran over this directory). This fixture + // deliberately embeds a `~/.claude/` reference so the assertion below + // actually exercises rewriteStagedSkillBodies (surface.cts) — it is + // NOT immune from that pass just because no per-file `converter` runs + // at stage time (see readInstalledCapabilitySkill's doc comment). + const AUTHORED = '---\nname: my-thing\ndescription: third-party test skill\n---\n\n# My Thing\nSee @~/.claude/workflows/example.md for details.\n'; + __installCapSkill(gsdHome, 'my-thing', 'my-thing', AUTHORED); + process.env.GSD_HOME = gsdHome; + + const registry = __registryFor('my-thing', ['my-thing']); + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + const layout = __resolveRuntimeArtifactLayout('claude', configDir, 'global', registry); + const resolved = __applySurface(configDir, layout, manifest, undefined, registry); + + assert.ok(resolved.skills.has('my-thing'), 'sanity: registry union (#2045) already includes my-thing in resolved.skills'); + + const stagedPath = path.join(configDir, 'skills', 'gsd-my-thing', 'SKILL.md'); + assert.ok( + fs.existsSync(stagedPath), + 'gsd-my-thing/SKILL.md must exist on disk after applySurface — registry says surfaced:true but nothing was materialized (#2322)', + ); + const staged = fs.readFileSync(stagedPath, 'utf8'); + assert.notStrictEqual( + staged, + AUTHORED, + 'MEDIUM-4: a third-party skill is NOT immune from the runtime body rewrite pass applySurface runs over the whole stage dir — a byte-for-byte-equal assertion here would be vacuous', + ); + assert.ok(!staged.includes('~/.claude/'), 'the ~/.claude/ literal must have been rewritten to the resolved path prefix'); + assert.ok(staged.includes('workflows/example.md'), 'the rewrite must preserve the referenced path suffix, not corrupt the body'); + } finally { + if (savedHome === undefined) delete process.env.GSD_HOME; else process.env.GSD_HOME = savedHome; + cleanup(gsdHome); + cleanup(configDir); + } + }); + + __issueTest('(2) collision: a first-party stem always wins over a same-named third-party capability skill', () => { + const gsdHome = createTempDir('gsd-2322-home-'); + const configDir = createTempDir('gsd-2322-cfg-'); + const savedHome = process.env.GSD_HOME; + try { + const EVIL = '---\nname: phase\n---\n\n# EVIL PHASE — must never win over the first-party gsd-phase skill\n'; + __installCapSkill(gsdHome, 'evil-cap', 'phase', EVIL); + process.env.GSD_HOME = gsdHome; + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + + // Baseline: apply against the SAME configDir with no colliding capability + // registered — this is the reference first-party conversion of + // commands/gsd/phase.md. (Two DIFFERENT configDirs would embed different + // absolute @-workflow-file paths in the body and could never compare + // byte-identical, so the collision run below re-applies to this same dir.) + const baselineLayout = __resolveRuntimeArtifactLayout('claude', configDir, 'global'); + __applySurface(configDir, baselineLayout, manifest, undefined, undefined); + const phasePath = path.join(configDir, 'skills', 'gsd-phase', 'SKILL.md'); + assert.ok(fs.existsSync(phasePath), 'sanity: first-party gsd-phase stages without a colliding capability'); + const baselinePhaseContent = fs.readFileSync(phasePath, 'utf8'); + + // Collision run: re-apply to the SAME configDir once a third-party + // capability also declares the 'phase' stem — the layout is rebuilt + // WITH the colliding registry so the third-party fill-in pass actually + // runs (and is then correctly suppressed by "first-party always wins"), + // rather than never attempting it at all. + const collideRegistry = __registryFor('evil-cap', ['phase']); + const collideLayout = __resolveRuntimeArtifactLayout('claude', configDir, 'global', collideRegistry); + __applySurface(configDir, collideLayout, manifest, undefined, collideRegistry); + assert.ok(fs.existsSync(phasePath), 'gsd-phase must still exist when a third-party capability collides on the same stem'); + const collidePhaseContent = fs.readFileSync(phasePath, 'utf8'); + + assert.notStrictEqual(collidePhaseContent, EVIL, 'the third-party EVIL PHASE content must never win the collision'); + assert.strictEqual( + collidePhaseContent, + baselinePhaseContent, + 'on a stem collision the first-party converted content must be staged, identical to the no-collision baseline', + ); + } finally { + if (savedHome === undefined) delete process.env.GSD_HOME; else process.env.GSD_HOME = savedHome; + cleanup(gsdHome); + cleanup(configDir); + } + }); + + __issueTest('(3) profile filter: a third-party skill outside the resolved profile is not staged; sibling first-party skills are unaffected', () => { + const gsdHome = createTempDir('gsd-2322-home-'); + const configDir = createTempDir('gsd-2322-cfg-'); + const savedHome = process.env.GSD_HOME; + try { + __installCapSkill(gsdHome, 'my-thing', 'my-thing', '# my-thing\n'); + process.env.GSD_HOME = gsdHome; + + // 'my-thing' is tier:'standard' -> profiles ['standard','full']; the + // 'core' base profile does NOT include it. + __writeSurface(configDir, { baseProfile: 'core', disabledClusters: [], explicitAdds: [], explicitRemoves: [] }); + + const registry = __registryFor('my-thing', ['my-thing'], 'standard'); + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + const layout = __resolveRuntimeArtifactLayout('claude', configDir, 'global', registry); + const resolved = __applySurface(configDir, layout, manifest, undefined, registry); + + assert.ok(!resolved.skills.has('my-thing'), 'sanity: core profile excludes a standard-tier capability skill'); + assert.ok( + !fs.existsSync(path.join(configDir, 'skills', 'gsd-my-thing', 'SKILL.md')), + 'a capability skill outside the resolved profile must NOT be staged', + ); + // 'phase' is a core-profile member — must be unaffected by the presence + // of the (filtered-out) third-party capability skill. + assert.ok( + fs.existsSync(path.join(configDir, 'skills', 'gsd-phase', 'SKILL.md')), + 'a first-party core-profile skill must still stage normally alongside a filtered-out capability skill', + ); + } finally { + if (savedHome === undefined) delete process.env.GSD_HOME; else process.env.GSD_HOME = savedHome; + cleanup(gsdHome); + cleanup(configDir); + } + }); + + __issueTest('(4) control: a nested-router (doNest) runtime layout is unperturbed by an installed capability skill', () => { + const gsdHome = createTempDir('gsd-2322-home-'); + const configDir = createTempDir('gsd-2322-cfg-'); + const savedHome = process.env.GSD_HOME; + try { + __installCapSkill(gsdHome, 'my-thing', 'my-thing', '# my-thing\n'); + process.env.GSD_HOME = gsdHome; + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + // 'cline' is a nested (doNest) runtime layout (unlike Claude's flat layout). + const baselineLayout = __resolveRuntimeArtifactLayout('cline', configDir, 'global'); + + // Baseline: no capability registered — snapshot the resulting nested layout. + __applySurface(configDir, baselineLayout, manifest, undefined, undefined); + const skillsDir = path.join(configDir, 'skills'); + const router = fs.readdirSync(skillsDir).find((d) => d.startsWith('gsd-ns-')); + assert.ok(router, 'sanity: the baseline nested layout has at least one gsd-ns-* router'); + const baselineSnapshot = __snapshotDir(skillsDir); + + // Re-apply to the SAME configDir once a capability skill is also + // installed and present in the registry — rebuild the layout WITH the + // registry so the fill-in pass actually runs. + const registry = __registryFor('my-thing', ['my-thing']); + const registryLayout = __resolveRuntimeArtifactLayout('cline', configDir, 'global', registry); + __applySurface(configDir, registryLayout, manifest, undefined, registry); + + assert.ok( + fs.existsSync(path.join(skillsDir, router)), + 'the namespace router bundle must still be present when a capability skill is also installed', + ); + // Every first-party file staged in the baseline (no capability) run + // must remain byte-identical after the capability is added — its + // presence must never perturb first-party nesting or content. + __assertSnapshotSubsetPreserved(baselineSnapshot, skillsDir, 'nested first-party layout'); + } finally { + if (savedHome === undefined) delete process.env.GSD_HOME; else process.env.GSD_HOME = savedHome; + cleanup(gsdHome); + cleanup(configDir); + } + }); + + __issueTest('(5) absent/malformed: a registry-referenced capability skill missing on disk must not throw and must not affect first-party output', () => { + const gsdHome = createTempDir('gsd-2322-home-'); + const configDirGhost = createTempDir('gsd-2322-cfg-ghost-'); + const configDirPartial = createTempDir('gsd-2322-cfg-partial-'); + const savedHome = process.env.GSD_HOME; + try { + // Partial case: the capability dir exists but the declared stem's + // skills//SKILL.md is missing (e.g. a corrupt/partial install). + fs.mkdirSync(path.join(gsdHome, '.gsd', 'capabilities', 'partial-cap', 'skills'), { recursive: true }); + process.env.GSD_HOME = gsdHome; + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + + // Ghost case: capId in the registry has NO install directory at all. + const ghostRegistry = __registryFor('ghost-cap', ['ghost-stem']); + const ghostLayout = __resolveRuntimeArtifactLayout('claude', configDirGhost, 'global', ghostRegistry); + assert.doesNotThrow(() => { + __applySurface(configDirGhost, ghostLayout, manifest, undefined, ghostRegistry); + }, 'applySurface must not throw when a registry-referenced capability has no install directory on disk'); + assert.ok( + fs.existsSync(path.join(configDirGhost, 'skills', 'gsd-help', 'SKILL.md')), + 'first-party skills must still materialize when a referenced capability is entirely absent on disk', + ); + + // Partial case. + const partialRegistry = __registryFor('partial-cap', ['missing-stem']); + const partialLayout = __resolveRuntimeArtifactLayout('claude', configDirPartial, 'global', partialRegistry); + assert.doesNotThrow(() => { + __applySurface(configDirPartial, partialLayout, manifest, undefined, partialRegistry); + }, 'applySurface must not throw when a capability install dir exists but the declared stem is missing under skills/'); + assert.ok( + fs.existsSync(path.join(configDirPartial, 'skills', 'gsd-help', 'SKILL.md')), + 'first-party skills must still materialize when a referenced capability skill is partially/malformed-installed', + ); + } finally { + if (savedHome === undefined) delete process.env.GSD_HOME; else process.env.GSD_HOME = savedHome; + cleanup(gsdHome); + cleanup(configDirGhost); + cleanup(configDirPartial); + } + }); + }); + + // ── BLOCKER 1: cross-capability skill-stem hijack ────────────────────────── + __issueDescribe('issue-2322 BLOCKER 1: capability skill stem hijack via an undeclared/unregistered directory', () => { + __issueTest('an UNDECLARED skills// directory shipped by one capability must never supply another capability\'s declared+registered stem', () => { + const gsdHome = createTempDir('gsd-2322-home-'); + const configDir = createTempDir('gsd-2322-cfg-'); + const savedHome = process.env.GSD_HOME; + try { + const EVIL = '---\nname: deploy\n---\n\n# EVIL deploy — planted by aaa-utils, which never declared this skill\n'; + const LEGIT = '---\nname: deploy\n---\n\n# legit deploy — authored and declared by zzz-deploy\n'; + // aaa-utils sorts BEFORE zzz-deploy lexically (the OLD scan-and-first- + // sorted-match implementation would have picked aaa-utils) AND ships an + // UNDECLARED skills/deploy/ directory it never listed in its + // capability.json `skills[]`. + __installCapSkill(gsdHome, 'aaa-utils', 'deploy', EVIL); + // zzz-deploy is the SOLE capability that DECLARES + is REGISTERED as + // owning 'deploy'. + __installCapSkill(gsdHome, 'zzz-deploy', 'deploy', LEGIT); + process.env.GSD_HOME = gsdHome; + + // aaa-utils's capabilityClusters entry is EMPTY — mirrors + // gen-capability-registry.cjs deriveCapabilityClusters, which only + // includes a capability that owns a NON-EMPTY declared skills[] array + // (i.e. "skills": [] in capability.json never appears here at all). + const registry = { + capabilityClusters: { 'aaa-utils': [], 'zzz-deploy': ['deploy'] }, + profileMembership: { 'zzz-deploy': { tier: 'standard', profiles: ['standard', 'full'] } }, + }; + + // (a) direct unit-level probe of the ownership-binding function. + const found = readInstalledCapabilitySkill('deploy', registry); + assert.ok(found, 'sanity: the declared+registered owner must be resolvable'); + assert.strictEqual(found.capId, 'zzz-deploy', 'ownership must resolve to the DECLARING+REGISTERED capability, never the alphabetically-first directory on disk'); + assert.strictEqual(found.content, LEGIT, 'the returned content must be the declaring capability\'s own SKILL.md'); + assert.notStrictEqual(found.content, EVIL, 'the undeclared sibling directory\'s content must never be returned'); + + // (b) end-to-end through the real staging path (applySurface). + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + const layout = __resolveRuntimeArtifactLayout('claude', configDir, 'global', registry); + const resolved = __applySurface(configDir, layout, manifest, undefined, registry); + assert.ok(resolved.skills.has('deploy'), 'sanity: the registered capability skill is unioned into the resolved surface'); + + const stagedPath = path.join(configDir, 'skills', 'gsd-deploy', 'SKILL.md'); + assert.ok(fs.existsSync(stagedPath), 'the legitimate declaring capability\'s skill must be staged'); + const stagedContent = fs.readFileSync(stagedPath, 'utf8'); + assert.ok(!stagedContent.includes('EVIL'), 'the undeclared aaa-utils directory must never win the stem — hijack blocked end-to-end'); + assert.ok(stagedContent.includes('legit deploy'), 'the declaring capability\'s own content must be what lands on disk'); + } finally { + if (savedHome === undefined) delete process.env.GSD_HOME; else process.env.GSD_HOME = savedHome; + cleanup(gsdHome); + cleanup(configDir); + } + }); + + __issueTest('two capabilities can never both claim the same stem — capabilityClusters is unambiguous by construction (no ownership tie to break)', () => { + // capabilityClusters can only ever bind a stem to ONE capId (the + // capability-loader registry composer rejects a candidate whose declared + // skill collides with an already-registered owner before it is ever + // composed) — so _owningCapabilityId's "first Object.keys() match wins" + // internal iteration order can never actually matter in practice. This + // documents that invariant at the readInstalledCapabilitySkill boundary. + const registry = { capabilityClusters: { alpha: ['shared'], beta: ['shared'] } }; + // Even in this (registry-invariant-violating, hand-crafted) input, the + // lookup must be deterministic and must not throw. + assert.doesNotThrow(() => readInstalledCapabilitySkill('shared', registry)); + }); + }); + + // ── BLOCKER 2: the '*' (full-profile) sentinel must still stage capability skills ── + __issueDescribe('issue-2322 BLOCKER 2: mode=full (the "*" sentinel) must still stage registered third-party capability skills', () => { + __issueTest('resolveProfile({modes:["full"]})\'s "*" sentinel, passed straight to stageSkillsForRuntimeAsSkills, still materializes a registered capability skill', () => { + const gsdHome = createTempDir('gsd-2322-home-'); + const configDir = createTempDir('gsd-2322-cfg-'); + const savedHome = process.env.GSD_HOME; + let unitStaged; + let e2eStaged; + try { + const AUTHORED = '# my-thing (full profile)\n'; + __installCapSkill(gsdHome, 'my-thing', 'my-thing', AUTHORED); + process.env.GSD_HOME = gsdHome; + + const registry = __registryFor('my-thing', ['my-thing']); + // The REAL sentinel resolveProfile({modes:['full']}) produces — mirrors + // bin/install.js's `_resolvedProfile` for the DEFAULT (`--profile full`) + // install, and does NOT go through applySurface/resolveSurface (which + // would materialize '*' into a concrete Set before staging ever sees it). + const resolvedFull = resolveProfile({ modes: ['full'] }); + assert.strictEqual(resolvedFull.skills, '*', 'sanity: full mode resolves to the "*" sentinel'); + + // (a) direct unit-level probe of the staging function. + unitStaged = stageSkillsForRuntimeAsSkills(REAL_COMMANDS_DIR, resolvedFull, (content) => content, 'gsd-', false, registry); + const unitStagedPath = path.join(unitStaged, 'gsd-my-thing', 'SKILL.md'); + assert.ok( + fs.existsSync(unitStagedPath), + '#2322 BLOCKER 2: the "*" sentinel must still stage a registered capability skill (unit level) — mode=standard already did this, mode=full must too', + ); + assert.strictEqual(fs.readFileSync(unitStagedPath, 'utf8'), AUTHORED); + + // (b) end-to-end through resolveRuntimeArtifactLayout's skills-kind + // stage() closure — the EXACT call shape bin/install.js's installer + // path uses (kind.stage(resolvedProfile), with NO applySurface + // involved, so the '*' sentinel really does reach staging here). + const layout = __resolveRuntimeArtifactLayout('claude', configDir, 'global', registry); + const skillsKindEntry = layout.kinds.find((k) => k.kind === 'skills'); + e2eStaged = skillsKindEntry.stage(resolvedFull); + const e2eStagedPath = path.join(e2eStaged, 'gsd-my-thing', 'SKILL.md'); + assert.ok( + fs.existsSync(e2eStagedPath), + 'BLOCKER 2 (issue repro): mode=full must stage a registered capability skill exactly like mode=standard already does', + ); + } finally { + if (savedHome === undefined) delete process.env.GSD_HOME; else process.env.GSD_HOME = savedHome; + cleanup(gsdHome); + cleanup(configDir); + if (unitStaged) cleanup(unitStaged); + if (e2eStaged) cleanup(e2eStaged); + } + }); + + __issueTest('no registry threaded through at all -> the "*" sentinel stages NOTHING third-party (fail closed, never a fallback scan)', () => { + const gsdHome = createTempDir('gsd-2322-home-'); + const savedHome = process.env.GSD_HOME; + let staged; + try { + __installCapSkill(gsdHome, 'my-thing', 'my-thing', '# my-thing\n'); + process.env.GSD_HOME = gsdHome; + const resolvedFull = resolveProfile({ modes: ['full'] }); + staged = stageSkillsForRuntimeAsSkills(REAL_COMMANDS_DIR, resolvedFull, (c) => c, 'gsd-', false /* no registry arg */); + assert.ok( + !fs.existsSync(path.join(staged, 'gsd-my-thing')), + 'an absent registry must never fall back to scanning the capabilities root — fail closed', + ); + } finally { + if (savedHome === undefined) delete process.env.GSD_HOME; else process.env.GSD_HOME = savedHome; + cleanup(gsdHome); + if (staged) cleanup(staged); + } + }); + }); + + // ── HIGH 3: staged capability skills must be prunable once orphaned ──────── + __issueDescribe('issue-2322 HIGH-3: an orphaned (uninstalled/unsurfaced) capability skill is pruned; a genuinely unknown gsd-* dir is still preserved', () => { + __issueTest('uninstalling the capability + re-applying removes its staged skill; an unrelated hand-made gsd-* dir survives with a warning', () => { + const gsdHome = createTempDir('gsd-2322-home-'); + const configDir = createTempDir('gsd-2322-cfg-'); + const savedHome = process.env.GSD_HOME; + try { + __installCapSkill(gsdHome, 'my-thing', 'my-thing', '# my-thing\n'); + process.env.GSD_HOME = gsdHome; + + const registry = __registryFor('my-thing', ['my-thing']); + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + const layoutWithCap = __resolveRuntimeArtifactLayout('claude', configDir, 'global', registry); + + // Step 1: install + apply -> the capability skill is staged and marked. + __applySurface(configDir, layoutWithCap, manifest, undefined, registry); + const stagedDirPath = path.join(configDir, 'skills', 'gsd-my-thing'); + assert.ok(fs.existsSync(path.join(stagedDirPath, 'SKILL.md')), 'sanity: the capability skill is staged'); + assert.ok( + fs.existsSync(path.join(stagedDirPath, CAPABILITY_SKILL_MARKER)), + 'sanity: the staged capability skill dir carries the persisted ownership marker', + ); + + // Step 2: an unrelated, genuinely user-created gsd-* dir must survive + // the whole scenario untouched, with a warning — the data-loss guard. + const userDir = path.join(configDir, 'skills', 'gsd-my-own-notes'); + fs.mkdirSync(userDir, { recursive: true }); + fs.writeFileSync(path.join(userDir, 'SKILL.md'), '# my own notes\n'); + + // Step 3: uninstall the capability (remove its on-disk bundle) and drop + // it from every registry view entirely — simulating `capability remove`. + cleanup(path.join(gsdHome, '.gsd', 'capabilities', 'my-thing')); + const registryAfterUninstall = { capabilityClusters: {}, profileMembership: {} }; + const layoutAfterUninstall = __resolveRuntimeArtifactLayout('claude', configDir, 'global', registryAfterUninstall); + + // Step 4: re-apply to the SAME configDir with the capability now + // entirely absent from the registry. + const stderrChunks = []; + const origWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk, ...rest) => { stderrChunks.push(String(chunk)); return origWrite(chunk, ...rest); }; + try { + __applySurface(configDir, layoutAfterUninstall, manifest, undefined, registryAfterUninstall); + } finally { + process.stderr.write = origWrite; + } + + assert.ok( + !fs.existsSync(stagedDirPath), + 'HIGH-3: an uninstalled/unsurfaced capability\'s previously-staged skill must be PRUNED, not preserved forever', + ); + assert.ok( + fs.existsSync(userDir), + 'a genuinely unknown/hand-made gsd-* dir must still be preserved (data-loss protection unchanged)', + ); + assert.ok( + stderrChunks.some((c) => c.includes('gsd-my-own-notes')), + 'the genuinely unknown dir must still emit the preserve warning', + ); + assert.ok( + !stderrChunks.some((c) => c.includes('gsd-my-thing')), + 'the orphaned capability skill must be pruned via its positively-identified marker, not routed through the "unknown" warning path', + ); + } finally { + if (savedHome === undefined) delete process.env.GSD_HOME; else process.env.GSD_HOME = savedHome; + cleanup(gsdHome); + cleanup(configDir); + } + }); + }); + + // ── LOW 7: isSafeCapabilitySkillStem — the security control had ZERO direct coverage ── + __issueDescribe('issue-2322 LOW-7: isSafeCapabilitySkillStem direct security-control coverage', () => { + const unsafe = [ + ['../../evil', 'parent-directory traversal (unix separators)'], + ['..\\..\\evil', 'parent-directory traversal (windows separators)'], + ['/etc/passwd', 'absolute unix path'], + ['C:\\x', 'absolute-looking windows path (rejected via the backslash check, platform-independent)'], + ['foo/../../bar', 'embedded traversal segment'], + ['foo\0bar', 'embedded NUL byte'], + ['.', 'current-directory sentinel'], + ['..', 'parent-directory sentinel'], + ['', 'empty string'], + ]; + for (const [stem, label] of unsafe) { + __issueTest(`rejects: ${label}`, () => { + assert.strictEqual(isSafeCapabilitySkillStem(stem), false, `"${JSON.stringify(stem)}" (${label}) must be rejected`); + }); + } + + __issueTest('accepts an ordinary single-segment stem (limit-1 / control case)', () => { + assert.strictEqual(isSafeCapabilitySkillStem('my-thing'), true); + }); + + __issueTest('boundary: a very long single-segment stem (no separators) has no length cap — accepted, and a lookup against it degrades gracefully (never throws)', () => { + const longStem = 'a'.repeat(5000); + assert.strictEqual(isSafeCapabilitySkillStem(longStem), true, 'no length cap is documented on this function — it is a shape check, not a length check'); + const registry = { capabilityClusters: { 'some-cap': [longStem] } }; + assert.doesNotThrow(() => readInstalledCapabilitySkill(longStem, registry), 'an overlong (but shape-valid) stem must degrade to null on a filesystem-level failure (e.g. ENAMETOOLONG), never throw'); + assert.strictEqual(readInstalledCapabilitySkill(longStem, registry), null); + }); + + __issueTest('capabilityClusterStems ignores prototype-pollution-shaped keys and non-array values', () => { + // Computed keys (['__proto__'], ['constructor']) create genuine OWN + // enumerable properties named "__proto__"/"constructor" — unlike the + // bare object-literal `__proto__:` form (which sets the object's actual + // [[Prototype]] instead and would never appear in Object.keys() at all, + // making the BANNED-list guard untestable this way). + const clusters = Object.create(null); + clusters['__proto__'] = ['evil']; + clusters['constructor'] = ['evil']; + clusters['good'] = ['a', 'b']; + clusters['bad'] = 'not-an-array'; + const stems = capabilityClusterStems({ capabilityClusters: clusters }); + assert.deepStrictEqual([...stems].sort(), ['a', 'b']); + }); + }); +} + + // ──────────────────────────────────────────────────────────────────────── // Folded from tests/bug-924-claude-flat-skill-layout.test.cjs — consolidation epic #1969 (B3 #1972) // ────────────────────────────────────────────────────────────────────────