* test(#2322): fail-first tests for third-party capability skill materialization Red phase: tests (1) and (6) fail — resolveSurface reports the third-party stem surfaced (#2045) but no SKILL.md is ever written to disk. The other four are controls that must keep holding: first-party-wins collision, profile-tier filter, nested-router layout unperturbed, and absent/malformed capability must not throw. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * fix(#2322): materialize installed third-party capability skills A capability could report installed:true, surfaced:true, active:true and still never exist as an invocable command. #2045 fixed the registry layer — resolveSurface unions registry.capabilityClusters into the resolved skill set — but the materialization layer never got the matching fix. stageSkillsForRuntimeAsSkills only ever read gsd-core's own bundled commands/gsd/*.md and silently skipped any stem it couldn't find there, so a third-party skill living at <GSD_HOME>/.gsd/capabilities/<id>/skills/<stem>/ was never copied. Registry said surfaced; disk had nothing. Installed capability skills are now staged alongside the first-party ones, copied verbatim (they are authored complete for their target runtime and need no converter). First-party stems always win a collision, the profile filter still applies, and an absent or malformed capability degrades rather than throwing. Security: capability.json's skills[] entries are validated only as non-empty non-reserved strings (capability-validator.cjs:503-514) — no path shape is enforced upstream — so stems are sanitized (rejecting separators, '..', absolute paths, NUL) with an independent isPathConfined check on both the read and write paths. A '../../evil' stem writes nothing outside the capability's own dir. Also fixes a defect this surfaced in pruneSkillDirs: a materialized capability skill dir has no first-party manifest entry, so every apply logged "preserving (user-owned or unknown)" for a live GSD-managed dir. The retained check now precedes the manifest gate; no deletion outcome changes, and genuinely unknown gsd-* dirs still warn and are preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * fix(#2322): address security review — bind skills to declaring capability, fix full profile An independent security review BLOCKED the first pass. Both blockers were mine. BLOCKER 1 (security): readInstalledCapabilitySkill scanned every capability dir and returned the first sorted match, never checking that a capability DECLARES the stem — ownership was inferred from attacker-controlled filesystem layout. Since install copies the whole bundle and the validator only checks DECLARED entries, a capability declaring `skills: []` could ship an undeclared skills/deploy/SKILL.md and win the `deploy` stem on sort order, supplying the agent-invocable instructions the user believed came from the registered capability. Stems are now bound to their owning capId via registry.capabilityClusters, and only that capability's dir is read. BLOCKER 2: the fill-in pass was gated `skills !== '*'` on the premise that applySurface materializes `full` into a concrete Set. True for applySurface — false for the installer, which is the default path: resolveProfile returns the '*' sentinel and bin/install.js passes it straight to staging. So #2322 survived on the default `full` profile, i.e. the fix didn't fix the reported bug. The registry is now plumbed to staging, and '*' stages all capability-cluster stems. Wiring this surfaced a second gap: the ADR-1239 imperative adapter (the primary install path) never threaded its registry either, which would have silently defeated the fix on the real default install. HIGH: staged capability skills were never prunable — pruneSkillDirs gates on the first-party manifest, so uninstalling a capability left its instructions live in the agent's context forever. Staged skills now carry a marker making them GSD-owned and prunable; genuinely unknown gsd-* dirs still warn and are preserved. MEDIUM: the "staged verbatim" claim was false — applySurface rewrites bodies over the whole stage dir. The tests asserted byte-equality and passed only because their fixtures contained no rewrite triggers. Claim dropped; tests now assert the rewrite against triggering content. LOW: isPathConfined is lexical, not realpath (symlink-defeatable, currently unreachable because install rejects symlinks) — comment corrected. The validator does not enforce non-empty, so isSafeCapabilitySkillStem is the sole defense, not a second layer — comment corrected and it now has traversal/NUL/absolute/empty test coverage (previously mutating it to `return true` left every test green). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * test(#2322): pin that the imperative adapter forwards a capability registry The delegation-args test deep-equalled the exact argv to installRuntimeArtifacts, so threading the composed capability registry through the ADR-1239 imperative adapter (required for #2322 — without it the default `full` install path never materializes third-party capability skills) failed it. The contract legitimately gained a parameter, so this is a stale-test correction, not a regression. Rather than deep-equalling the whole composed registry (brittle — it embeds the full agent/profile map), the test pins the leading args exactly and asserts only that a registry-shaped value is forwarded. That still fails if the adapter stops threading it, which is the regression the test exists to catch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * docs(#2322): backfill PR number 2340 into changeset Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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<st
|
||||
*
|
||||
* Ownership criteria:
|
||||
* - Non-empty prefix (e.g. 'gsd-'): dir name starts with that prefix AND
|
||||
* appears in the manifest (manifest membership is required). Dirs that match
|
||||
* the prefix but are NOT in the manifest are treated as user-owned and
|
||||
* EITHER appears in the manifest (first-party membership) OR carries the
|
||||
* persisted `CAPABILITY_SKILL_MARKER` file (#2322 HIGH-3: a third-party
|
||||
* capability skill, self-certifying and independent of current registry
|
||||
* state — so an uninstalled/unsurfaced capability's stale skill is still
|
||||
* prunable even though it no longer appears in any registry view). Dirs
|
||||
* that match the prefix but satisfy NEITHER are treated as user-owned and
|
||||
* preserved — this prevents data loss for user-created gsd-* directories.
|
||||
* A warning is written to stderr when such a dir is encountered.
|
||||
* - Empty prefix (Hermes): dir name appears as a canonical skill stem in the
|
||||
* manifest. User dirs not in the manifest are preserved.
|
||||
* manifest. User dirs not in the manifest are preserved. (Hermes does not
|
||||
* yet stage third-party capability skills, so the marker check does not
|
||||
* apply on this path.)
|
||||
* - Empty prefix without manifest, or manifest not a Map: conservative; no
|
||||
* dirs are removed.
|
||||
*
|
||||
@@ -468,20 +478,48 @@ function pruneSkillDirs(skillsDir: string, retainedNames: Set<string>, 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);
|
||||
|
||||
Reference in New Issue
Block a user