From 33938055417a0bba02c7aec90faf640fb6ac9593 Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Mon, 14 Sep 2026 18:20:58 +0800 Subject: [PATCH 1/2] fix: pack-scoped skill ids, dangling-link repair, session-scoped skills via SDK plugins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes to how a pack's skills reach a session, found while auditing why the only pipeline run to date died at its sixth phase with "Unknown command: /adversary". **Pack-scoped registry ids.** The daemon-wide skill/gate registries were keyed by bare id, so installing a second pack that also declared `review`, `ship`, or `tests_pass` replaced the first pack's entries last-wins — the boot log announced it — and a run from the first pack drove the second pack's skill. Packs now register under `/` (new `scoped.ts`); the engine, the skill phase kind, and create-time validation resolve the run's own pack entry first and fall back to the bare id, which is how built-in gates (`always`, `manual`) and explicit-`phases` plans keep working unchanged. Phase defs keep the ids as authored, so CLI output and the Pack Browser are untouched. **Dangling-link repair.** An earlier loader linked registry skills from a temp clone under `/tmp`; after the cache moved to `~/.codeoid/packs/` those links pointed at nothing. `existsSync` is false for a dangling symlink, so `#linkSkills` tried to create it, hit EEXIST, warned, and left the skill broken on every install and trust thereafter. A dangling link is now unlinked and relinked; a real directory or a live link is still never touched. **Session-scoped skills.** Until now a trusted pack's skills reached a session one way: symlinked into `~/.claude/skills`, visible to every Claude Code session on the machine. That is the wrong scope for a machine whose `~/.claude` is owned by something else. New config `pipeline.skillScope` (`global` default, `session` opt-in): under `session` nothing is linked; PackService synthesizes a Claude-Code-plugin-shaped dir per registry (`~/.codeoid/plugins//` with a manifest and `skills → /skills`), `resolveActivation()` carries it as `skillsPluginDir`, Session passes it per turn as `TurnOpts.pluginDirs`, and the Claude provider hands it to the SDK's `plugins` option — inside the same rebuild guard as the prompt append and skill grants, since a pipeline swaps activations between phases. The read sandbox and the skill-command grant scan include the plugin tier so `!`…`` substitutions keep working. Same trust rule as linking. Verified against the real binary: a local plugin's skills resolve both bare (`/spec`) and namespaced (`/:spec`), and a plugin whose `skills` entry is a symlink is discovered — the exact layout synthesized here — so pack `command:` values need no rewrite. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Yash Datta --- CHANGELOG.md | 38 ++++++ docs/pack-loading.md | 31 ++++- src/config.ts | 23 +++- src/daemon/pipeline/engine.ts | 5 +- src/daemon/pipeline/manager.ts | 13 +- src/daemon/pipeline/pack-service.test.ts | 76 ++++++++++- src/daemon/pipeline/pack-service.ts | 155 +++++++++++++++++++++-- src/daemon/pipeline/pack.test.ts | 47 ++++++- src/daemon/pipeline/pack.ts | 9 +- src/daemon/pipeline/scoped.ts | 37 ++++++ src/daemon/pipeline/skill-kind.test.ts | 27 +++- src/daemon/pipeline/skill-kind.ts | 4 +- src/daemon/providers/claude/index.ts | 70 ++++++++-- src/daemon/providers/interface.ts | 9 ++ src/daemon/session-manager.ts | 3 + src/daemon/session.ts | 11 ++ src/tests/provider-claude.test.ts | 28 ++++ 17 files changed, 546 insertions(+), 40 deletions(-) create mode 100644 src/daemon/pipeline/scoped.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 99e5300e..594534dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,44 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `.cancel`, all gated on `settings:write`. Claude is wired today; the mechanism is per-backend and the others follow. +- **Session-scoped pack skills** (`pipeline.skillScope: "session"`). Until now + a trusted pack's registry skills reached a session one way only: symlinked + into `~/.claude/skills`, where every Claude Code session on the machine — not + just codeoid's — discovered them. On a machine whose `~/.claude` is owned by + something else (an org bundle linked by hand, another toolkit) that is the + wrong scope. With `skillScope: "session"` nothing is linked; codeoid + synthesizes a Claude-Code-plugin-shaped directory per registry + (`~/.codeoid/plugins//`: a manifest plus `skills → /skills`) + and hands it to the SDK's `plugins` option for pack-activated sessions and + pipeline phases, so the methodology's skills exist inside codeoid runs and + nowhere else. Plugin skills resolve both bare (`/spec`) and namespaced + (`/:spec`), so pack `command:` values are unchanged; the same trust + rule applies (an untrusted pack contributes no runnable skills either way); + the read sandbox and the skill-command grants scan the plugin tier too. The + default stays `global`. Non-Claude backends ignore the plugin dirs, as they + already ignore pack subagents. (docs/pack-loading.md §3a) + +### Fixed + +- **Two installed packs declaring the same skill or gate id overwrote each + other.** Registries were daemon-wide and keyed by bare id, so installing a + second pack that also declared `review`, `ship`, or `tests_pass` replaced the + first's entries last-wins (the boot log said so: `skill "review" already + registered — overwriting`), and a run from the first pack then drove the + second pack's skill. Packs now register under `/`; the engine, the + skill phase kind, and create-time validation resolve a run's own pack entry + first and fall back to the bare id, so built-in gates (`always`, `manual`) and + explicit-`phases` plans keep resolving exactly as before. Phase defs, CLI + output, and the web Pack Browser still show the ids as authored. + +- **A dangling skill symlink blocked that skill from ever being linked again.** + An older loader linked registry skills from a temp clone under `/tmp`; after + the cache moved, those links pointed at nothing. `existsSync` is false for a + dangling link, so `#linkSkills` tried to create it, hit `EEXIST`, warned, and + left the skill broken on every subsequent install and trust. A dangling link + is now repaired in place; a real directory or a live link is still never + touched. + ## [0.4.0] - 2026-07-29 codeoid moves to the Highflame npm org. npm has no way to transfer a package diff --git a/docs/pack-loading.md b/docs/pack-loading.md index a1440e7a..cdab5d6d 100644 --- a/docs/pack-loading.md +++ b/docs/pack-loading.md @@ -47,7 +47,7 @@ pipeline runtime on. It owns: | `refresh(name?)` | `git pull` a cached registry | | `available()` | packs found across caches, not yet installed | | `installed()` | loaded packs + metadata + trust + selected flag + status | -| `install(ref, { trusted })` | resolve a pack (registry `id` or explicit dir) → `loadPack` → persist to `config.pipeline.packs` → `installPack` into the live manager (if any) → link the registry's `skills/` into `~/.claude/skills` so the pack is *runnable* | +| `install(ref, { trusted })` | resolve a pack (registry `id` or explicit dir) → `loadPack` → persist to `config.pipeline.packs` → `installPack` into the live manager (if any) → under `skillScope: global`, link the registry's `skills/` into `~/.claude/skills` so the pack is *runnable* (see §3a) | | `remove(id)` | unregister from the manager + drop from config | | `trust(id, trusted)` | update config trust + reload the pack at the new trust | | `select(id)` | set `config.pipeline.defaultPack` | @@ -56,6 +56,35 @@ pipeline runtime on. It owns: skill/review gates work, but its shell `command` gates fail closed until an explicit `trust`. Matches the sandbox zero-standing-privilege posture. +### 3a. Skill scope — machine-wide symlinks or per-session plugins + +A trusted pack's registry `skills/` can reach a session two ways, chosen by +`config.pipeline.skillScope`: + +| `skillScope` | How the skills reach a session | Who else sees them | +| --- | --- | --- | +| `global` (default) | `#linkSkills` symlinks each `skills/` into `~/.claude/skills` on install / trust / refresh (additive; a **dangling** link left by an older loader is repaired, a real dir or live link is never touched) | every Claude Code session on the machine — the user's own same-named skills win collisions | +| `session` | nothing is linked; `resolveActivation()` returns `skillsPluginDir`, a synthesized Claude-Code-plugin dir (`~/.codeoid/plugins//` = `.claude-plugin/plugin.json` + `skills → /skills`) that the Claude backend passes to the SDK `plugins` option for that session's turns | only pack-activated codeoid sessions and pipeline phases | + +Both scopes apply the same trust rule (an untrusted pack contributes no +runnable skills either way), and both scan the skills for their `!`…`` +substitutions so the command grants (#233) and the read sandbox work +identically. Plugin skills resolve both bare (`/spec`) and namespaced +(`/:spec`), so pack `command:` values are unchanged. + +`session` is the scope for a machine whose `~/.claude` is owned by something +else (an org bundle symlinked by hand, another toolkit): the methodology's +skills exist inside codeoid runs and nowhere else. Switching an existing +machine from `global` to `session` does not remove links already made — delete +the `~/.claude/skills/` symlinks that point into `~/.codeoid/packs/` if +you want them gone. Non-Claude backends ignore `pluginDirs` today, exactly as +they ignore pack subagents. + +Registry skills and gates are registered under `/` (`scoped.ts`) +so two installed packs declaring the same bare id (`review`, `ship`, +`tests_pass`) coexist; a run resolves its own pack's entry first and falls back +to the bare id for built-in gates and explicit-`phases` plans. + Persistence goes through one shared config mutator (`mutateConfigFile`) that read → mutates → validates against `RootSchema` → atomically writes `0o600` — reusing the settings-store path so config integrity is enforced in one place. diff --git a/src/config.ts b/src/config.ts index 6d7558b9..d390790d 100644 --- a/src/config.ts +++ b/src/config.ts @@ -770,6 +770,15 @@ const PipelineSchema = z modelTiers: z.record(z.string().min(1).max(64), ModelBindingSchema).default({}), // Key = "/" — both ids are ≤64 chars, plus the slash. modelRoles: z.record(z.string().min(1).max(129), ModelBindingSchema).default({}), + /** + * How a trusted pack's registry skills reach a session (docs/pack-loading.md + * §3a). `global` (default): symlinked into `~/.claude/skills`, so every + * Claude Code session on the machine sees them. `session`: never linked; + * exposed as a per-session SDK plugin only inside pack-activated codeoid + * sessions and pipeline runs — the machine-wide `~/.claude` stays whatever + * the operator manages by hand. + */ + skillScope: z.enum(["global", "session"]).default("global"), packs: z .array( z.object({ @@ -794,7 +803,15 @@ const PipelineSchema = z ) .default([]), }) - .default({ enabled: true, defaultPack: null, packs: [], registries: [], modelTiers: {}, modelRoles: {} }); + .default({ + enabled: true, + defaultPack: null, + packs: [], + registries: [], + modelTiers: {}, + modelRoles: {}, + skillScope: "global", + }); /** * Push notifications (docs/push.md). When a session blocks on a tool approval, @@ -1070,6 +1087,10 @@ export interface CodeoidConfig { * (schema default {}). */ modelTiers?: Record; modelRoles?: Record; + /** `global` (default) symlinks trusted pack skills machine-wide; `session` + * exposes them only inside pack-activated sessions via an SDK plugin + * (docs/pack-loading.md §3a). Optional in the type; loadConfig defaults it. */ + skillScope?: "global" | "session"; }; /** * Per-backend provider settings. Optional in the type so hand-built test diff --git a/src/daemon/pipeline/engine.ts b/src/daemon/pipeline/engine.ts index 2c9be438..1b1c7179 100644 --- a/src/daemon/pipeline/engine.ts +++ b/src/daemon/pipeline/engine.ts @@ -22,6 +22,7 @@ import type { } from "./interface"; import { isTerminal } from "./interface"; import { errMessage } from "./errors"; +import { resolveScoped } from "./scoped"; /** Defensive cap against a mis-authored retry loop (each retry is one step). */ const MAX_STEPS = 10_000; @@ -199,7 +200,9 @@ export class PipelineEngine { phase: PhaseDef, at: "entry" | "exit", ): Promise { - const g = this.#registries.gates.resolve(id); + // The run's own pack entry first (`/`), then a bare built-in + // (`always` / `manual`) or directly registered gate — see scoped.ts. + const g = resolveScoped(this.#registries.gates, pipeline.packId, id); if (!g) return { pass: false, reason: `unknown ${at} gate "${id}"` }; try { return await g.evaluate({ pipeline: clone(pipeline), phase }); diff --git a/src/daemon/pipeline/manager.ts b/src/daemon/pipeline/manager.ts index b3c33b29..61e7808d 100644 --- a/src/daemon/pipeline/manager.ts +++ b/src/daemon/pipeline/manager.ts @@ -22,6 +22,7 @@ import type { Pack, PhaseDef, PipelineRegistries, PipelineState } from "./interf import { isTerminal } from "./interface"; import { createRegistries } from "./registry"; import type { PhaseRunner } from "./runner"; +import { hasScoped } from "./scoped"; import { makeSkillPhaseKind } from "./skill-kind"; import type { PipelineStore } from "./store"; @@ -106,7 +107,7 @@ export class PipelineManager { create(opts: CreatePipelineOpts): PipelineState { const { phases: plan, pack } = this.#resolvePhases(opts); const phases = this.#bindModels(plan, pack, opts); - this.#validate(phases); + this.#validate(phases, pack?.id); const ts = Date.now(); const state: PipelineState = { id: randomUUID(), @@ -427,7 +428,9 @@ export class PipelineManager { }); } - #validate(phases: PhaseDef[]): void { + /** `packId` scopes gate/skill lookups to the pack the plan came from + * (`/` first, bare second — scoped.ts); absent for explicit plans. */ + #validate(phases: PhaseDef[], packId?: string): void { if (phases.length === 0) throw new Error("pipeline must declare at least one phase"); const seen = new Set(); for (const p of phases) { @@ -436,15 +439,15 @@ export class PipelineManager { if (!this.#registries.phases.has(p.kind)) { throw new Error(`phase "${p.id}": unknown kind "${p.kind}"`); } - if (p.gate && !this.#registries.gates.has(p.gate)) { + if (p.gate && !hasScoped(this.#registries.gates, packId, p.gate)) { throw new Error(`phase "${p.id}": unknown gate "${p.gate}"`); } - if (p.entryGate && !this.#registries.gates.has(p.entryGate)) { + if (p.entryGate && !hasScoped(this.#registries.gates, packId, p.entryGate)) { throw new Error(`phase "${p.id}": unknown entry gate "${p.entryGate}"`); } if (p.kind === "skill") { if (!p.skill) throw new Error(`phase "${p.id}": kind "skill" requires a skill id`); - if (!this.#registries.skills.has(p.skill)) { + if (!hasScoped(this.#registries.skills, packId, p.skill)) { throw new Error(`phase "${p.id}": unknown skill "${p.skill}"`); } } diff --git a/src/daemon/pipeline/pack-service.test.ts b/src/daemon/pipeline/pack-service.test.ts index f6370318..32596d3d 100644 --- a/src/daemon/pipeline/pack-service.test.ts +++ b/src/daemon/pipeline/pack-service.test.ts @@ -11,7 +11,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import type { ModelBindingConfig } from "./binding"; import type { Pack } from "./interface"; -import { PackService, registryNameFromUrl, type PackServiceConfig } from "./pack-service"; +import { PackService, registryNameFromUrl, type PackServiceConfig, type SkillScope } from "./pack-service"; const tmps: string[] = []; function tmp(): string { @@ -82,6 +82,8 @@ function fakeSink() { function makeService(opts: { cacheDir: string; skillsDir?: string; + skillScope?: SkillScope; + pluginsDir?: string; fixture?: string; // a prepared registry dir the fake `git clone` copies in sink?: ReturnType; initial?: Partial; @@ -97,6 +99,8 @@ function makeService(opts: { }, cacheDir: opts.cacheDir, skillsDir: opts.skillsDir, + skillScope: opts.skillScope, + pluginsDir: opts.pluginsDir, manager: opts.sink ? () => opts.sink! : undefined, modelConfig: opts.modelConfig, persist: (s) => persisted.push(structuredClone(s)), @@ -306,6 +310,76 @@ describe("install / trust / select / remove", () => { expect(existsSync(join(skillsDir, "spec"))).toBe(true); }); + test("skill-linking repairs a DANGLING symlink (but still never clobbers a live one)", async () => { + const fixture = tmp(); + writeRegistry(fixture, ["p"], ["spec", "review"]); + const skillsDir = join(tmp(), "skills"); + mkdirSync(skillsDir, { recursive: true }); + // An earlier loader linked `spec` from a temp clone that no longer exists — + // existsSync() is false for it, so the old code hit EEXIST on every install. + symlinkSync(join(tmp(), "gone-clone", "skills", "spec"), join(skillsDir, "spec"), "dir"); + // `review` is a LIVE link to something else the operator manages — untouched. + const theirs = join(tmp(), "theirs"); + mkdirSync(theirs, { recursive: true }); + symlinkSync(theirs, join(skillsDir, "review"), "dir"); + const cacheDir = join(tmp(), "c"); + const { svc } = makeService({ cacheDir, skillsDir, fixture }); + await svc.addRegistry({ url: "https://github.com/a/reg.git" }); + svc.install({ packId: "p", trusted: true }); + const fs = require("node:fs"); + // The dangling `spec` now points at the registry cache; `review` is untouched. + expect(fs.realpathSync(join(skillsDir, "spec"))).toBe(fs.realpathSync(join(cacheDir, "reg", "skills", "spec"))); + expect(fs.realpathSync(join(skillsDir, "review"))).toBe(fs.realpathSync(theirs)); + }); + + test("skillScope: session — links nothing, exposes a per-session skill plugin instead", async () => { + const fixture = tmp(); + writeRegistry(fixture, ["p"], ["spec", "review"]); + const skillsDir = join(tmp(), "skills"); + const pluginsDir = join(tmp(), "plugins"); + const { svc } = makeService({ cacheDir: join(tmp(), "c"), skillsDir, pluginsDir, skillScope: "session", fixture }); + await svc.addRegistry({ url: "https://github.com/a/reg.git" }); + svc.install({ packId: "p", trusted: true }); + svc.trust("p", true); + await svc.refreshRegistry("reg"); + // Nothing reaches the machine-wide skills dir on any of install / trust / refresh. + expect(existsSync(join(skillsDir, "spec"))).toBe(false); + expect(existsSync(join(skillsDir, "review"))).toBe(false); + // The activation carries a Claude-Code-plugin-shaped dir for the registry. + const act = svc.resolveActivation("p"); + expect(act.skillsPluginDir).toBe(join(pluginsDir, "reg")); + const manifest = JSON.parse(readFileSync(join(pluginsDir, "reg", ".claude-plugin", "plugin.json"), "utf8")); + expect(manifest.name).toBe("reg"); + // `skills` inside the plugin resolves to the registry cache's skills. + expect(existsSync(join(pluginsDir, "reg", "skills", "spec", "SKILL.md"))).toBe(true); + expect(existsSync(join(pluginsDir, "reg", "skills", "review", "SKILL.md"))).toBe(true); + // Idempotent: a second activation neither throws nor changes the dir. + expect(svc.resolveActivation("p").skillsPluginDir).toBe(join(pluginsDir, "reg")); + }); + + test("skillScope: session — an UNTRUSTED pack gets no plugin (declaring is not executing)", async () => { + const fixture = tmp(); + writeRegistry(fixture, ["p"], ["spec"]); + const pluginsDir = join(tmp(), "plugins"); + const { svc } = makeService({ cacheDir: join(tmp(), "c"), pluginsDir, skillScope: "session", fixture }); + await svc.addRegistry({ url: "https://github.com/a/reg.git" }); + svc.install({ packId: "p" }); // untrusted + expect(svc.resolveActivation("p").skillsPluginDir).toBeUndefined(); + expect(existsSync(join(pluginsDir, "reg"))).toBe(false); + }); + + test("skillScope: global (default) — links as before and carries no plugin dir", async () => { + const fixture = tmp(); + writeRegistry(fixture, ["p"], ["spec"]); + const skillsDir = join(tmp(), "skills"); + const { svc } = makeService({ cacheDir: join(tmp(), "c"), skillsDir, fixture }); + await svc.addRegistry({ url: "https://github.com/a/reg.git" }); + svc.install({ packId: "p", trusted: true }); + expect(svc.skillScope).toBe("global"); + expect(existsSync(join(skillsDir, "spec"))).toBe(true); + expect(svc.resolveActivation("p").skillsPluginDir).toBeUndefined(); + }); + test("select sets the default pack (and rejects an uninstalled id)", () => { const dir = join(tmp(), "p"); writePack(dir, "sel"); diff --git a/src/daemon/pipeline/pack-service.ts b/src/daemon/pipeline/pack-service.ts index 9ff91aad..9601acc1 100644 --- a/src/daemon/pipeline/pack-service.ts +++ b/src/daemon/pipeline/pack-service.ts @@ -14,9 +14,20 @@ * truth for packs at runtime — this service is. */ -import { existsSync, lstatSync, mkdirSync, readdirSync, readFileSync, statSync, symlinkSync } from "node:fs"; +import { + existsSync, + lstatSync, + mkdirSync, + readdirSync, + readFileSync, + readlinkSync, + statSync, + symlinkSync, + unlinkSync, + writeFileSync, +} from "node:fs"; import { homedir } from "node:os"; -import { join, resolve } from "node:path"; +import { dirname, join, resolve } from "node:path"; import type { AvailablePackWire, PackWire, RegistryWire } from "@highflame/codeoid-protocol"; import { type ModelBindingConfig, resolveBinding } from "./binding"; import type { Pack } from "./interface"; @@ -38,8 +49,18 @@ export interface PackActivation { * postures) don't have to carry an empty map. */ roles?: Record; subagents: PackSubagent[]; + /** Session-scoped skill plugin (`pipeline.skillScope: "session"`): a Claude- + * Code-plugin-shaped directory exposing the pack's registry `skills/` to THIS + * session only — nothing is linked into `~/.claude/skills`. Absent under the + * default global scope (skills are symlinked machine-wide instead), for an + * untrusted pack (declaring is not executing), and for a dir-installed pack + * (no registry to expose). */ + skillsPluginDir?: string; } +/** How a trusted pack's registry skills reach a session (docs/pack-loading.md §3a). */ +export type SkillScope = "global" | "session"; + /** The minimal slice of PipelineManager the service needs to (un)register packs * for live effect — kept structural so tests can inject a fake. `installPack` * registers the pack's skills + gates AND indexes it (so pipeline.create({pack}) @@ -94,6 +115,17 @@ export interface PackServiceDeps { cacheDir?: string; /** Where to link a registry's runnable skills (default: ~/.claude/skills). */ skillsDir?: string; + /** + * `global` (default): symlink a trusted pack's registry skills into + * `skillsDir`, where EVERY Claude Code session on the machine discovers them. + * `session`: never link; synthesize a per-registry plugin dir instead and hand + * it to pack-activated sessions only (PackActivation.skillsPluginDir → the SDK + * `plugins` option), so a methodology's skills exist inside codeoid runs and + * nowhere else. Same trust rule either way. + */ + skillScope?: SkillScope; + /** Where session-scoped skill plugins are synthesized (default: `/../plugins`). */ + pluginsDir?: string; /** Run git (injectable). Default: `git` via Bun.spawn. */ git?: (args: string[], cwd?: string) => Promise; /** The operator's model maps (config `pipeline.modelTiers`/`modelRoles`) — @@ -120,6 +152,16 @@ async function defaultGit(args: string[], cwd?: string): Promise { return { ok: code === 0, stderr: stderr.trim() }; } +/** A symlink whose target no longer exists (`existsSync` follows links, so it + * is false; `lstatSync` sees the link itself). */ +function isDanglingSymlink(path: string): boolean { + try { + return lstatSync(path).isSymbolicLink() && !existsSync(path); + } catch { + return false; + } +} + /** Derive a cache-safe registry name from a git URL (last path segment, minus * `.git`). `git@github.com:highflame-ai/ai-factory.git` → `ai-factory`. */ export function registryNameFromUrl(url: string): string { @@ -137,6 +179,8 @@ export class PackService { #defaultPack: string | null; #cacheDir: string; #skillsDir: string; + #skillScope: SkillScope; + #pluginsDir: string; #git: (args: string[], cwd?: string) => Promise; #persist?: (state: PackServiceConfig) => void; #manager: PackServiceDeps["manager"]; @@ -148,6 +192,8 @@ export class PackService { this.#defaultPack = deps.config.defaultPack; this.#cacheDir = deps.cacheDir ?? join(homedir(), ".codeoid", "packs"); this.#skillsDir = deps.skillsDir ?? join(homedir(), ".claude", "skills"); + this.#skillScope = deps.skillScope ?? "global"; + this.#pluginsDir = deps.pluginsDir ?? join(dirname(this.#cacheDir), "plugins"); this.#git = deps.git ?? defaultGit; this.#persist = deps.persist; this.#manager = deps.manager; @@ -213,8 +259,15 @@ export class PackService { } } + /** The active skill scope (read by tests + `pack list` rendering). */ + get skillScope(): SkillScope { + return this.#skillScope; + } + // Two steps required: pull alone leaves the live pipeline stale (issue #236). // Skills re-linked only for trusted packs — invariant from install()/trust() (issue #233). + // Under session scope nothing is linked: the plugin dir points at the cache, + // so a pull is already visible to the next pack-activated turn. async refreshRegistry(name?: string): Promise { await this.refresh(name); const mgr = this.#manager?.(); @@ -233,7 +286,7 @@ export class PackService { if (existsSync(root)) skillRoots.add(root); } } - for (const root of skillRoots) this.#linkSkills(root); + if (this.#skillScope === "global") for (const root of skillRoots) this.#linkSkills(root); } // ── Discovery ─────────────────────────────────────────────────────────────── @@ -399,7 +452,11 @@ export class PackService { // gate path already requires — the exact "declaring is not executing" model // in loadPack. Only trusted packs get linked; an untrusted pack still // installs and indexes, it just contributes no runnable slash-skills. - if (registryRoot && trusted) this.#linkSkills(registryRoot); + // + // Under `skillScope: "session"` nothing is linked here at all: the skills + // reach pack-activated sessions through resolveActivation().skillsPluginDir + // (same trust rule, applied there). + if (registryRoot && trusted && this.#skillScope === "global") this.#linkSkills(registryRoot); return this.installed(); } @@ -427,7 +484,7 @@ export class PackService { // and the pack silently stays half-installed. (Toggling OFF leaves the // links; removing them belongs to `remove`, and unlinking live skills // mid-session is out of scope here.) - if (trusted && entry.registry) { + if (trusted && entry.registry && this.#skillScope === "global") { const root = this.#cachePath(entry.registry); if (existsSync(root)) this.#linkSkills(root); } @@ -470,7 +527,22 @@ export class PackService { // Subagents ship at the registry root's `agents/` dir (like skills). A // local-dir install has no registry → no subagents. const subagents = entry.registry ? loadSubagents(join(this.#cachePath(entry.registry), "agents")) : []; - return { id: loaded.id, constitution: loaded.constitution, role, roleName, roles: loaded.roles, subagents }; + // Session-scoped skills: the same trust rule as #linkSkills (an untrusted + // pack's `!`…`` frontmatter must not become runnable shell), delivered as a + // per-session plugin instead of a machine-wide symlink. + const skillsPluginDir = + this.#skillScope === "session" && entry.trusted && entry.registry + ? this.#ensureSkillPlugin(entry.registry) + : undefined; + return { + id: loaded.id, + constitution: loaded.constitution, + role, + roleName, + roles: loaded.roles, + subagents, + ...(skillsPluginDir ? { skillsPluginDir } : {}), + }; } // ── internals ─────────────────────────────────────────────────────────────── @@ -490,7 +562,14 @@ export class PackService { /** Symlink each `skills/` in a registry into the skills dir — additively * (never clobbers an existing skill). Best-effort: a link failure is logged, - * not fatal, since it only affects a pack's *runnability*, not its install. */ + * not fatal, since it only affects a pack's *runnability*, not its install. + * + * The one existing entry it WILL replace is a dangling symlink: an earlier + * loader linked skills from a temp clone (`/tmp/packsvc-…`) that no longer + * exists, and `existsSync(to)` is false for such a link — so the old code + * tried `symlinkSync`, got EEXIST, warned, and the skill stayed broken on + * every install/trust forever. A dangling link is nobody's skill; relinking + * it is the repair, not a clobber. A real dir or a live link is never touched. */ #linkSkills(registryRoot: string): string[] { const src = join(registryRoot, "skills"); if (!existsSync(src)) return []; @@ -508,7 +587,13 @@ export class PackService { // an untrusted registry must NOT be treated as a directory and propagated // into the host skills dir (it could point at /etc, ~/.ssh, …). A real // directory links; a symlink is skipped. - if (!lstatSync(from).isDirectory() || existsSync(to)) continue; + if (!lstatSync(from).isDirectory()) continue; + if (isDanglingSymlink(to)) { + unlinkSync(to); + console.log(`[packs] repaired dangling skill link "${name}" → ${from}`); + } else if (existsSync(to)) { + continue; + } symlinkSync(from, to, "dir"); linked.push(name); } catch (e) { @@ -518,6 +603,60 @@ export class PackService { return linked; } + /** + * Session-scoped skills (`skillScope: "session"`): materialize a Claude-Code- + * plugin-shaped directory for a registry — `.claude-plugin/plugin.json` naming + * the registry, plus `skills` → `/skills` — and return it for the SDK + * `plugins` option. Idempotent and self-repairing (a stale `skills` link is + * re-pointed). Registries are data-only, so the manifest is synthesized here + * rather than required of them. Plugin skills resolve both bare (`/spec`) and + * namespaced (`/:spec`), so pack `command:` values need no rewrite. + * Returns undefined when the registry ships no `skills/` or the dir can't be + * written (logged) — the activation then simply carries no plugin. + */ + #ensureSkillPlugin(registry: string): string | undefined { + const skillsSrc = join(this.#cachePath(registry), "skills"); + if (!existsSync(skillsSrc)) return undefined; + const pluginDir = join(this.#pluginsDir, registry); + try { + mkdirSync(join(pluginDir, ".claude-plugin"), { recursive: true }); + const manifestPath = join(pluginDir, ".claude-plugin", "plugin.json"); + const manifest = `${JSON.stringify( + { + name: registry, + description: `codeoid pack registry "${registry}" — skills exposed per session (pipeline.skillScope: session)`, + version: "0.0.0", + }, + null, + 2, + )}\n`; + if (!existsSync(manifestPath) || readFileSync(manifestPath, "utf8") !== manifest) { + writeFileSync(manifestPath, manifest); + } + const link = join(pluginDir, "skills"); + let current: ReturnType | undefined; + try { + current = lstatSync(link); + } catch { + current = undefined; + } + if (current?.isSymbolicLink()) { + if (readlinkSync(link) !== skillsSrc || !existsSync(link)) unlinkSync(link); + else return pluginDir; + } else if (current) { + // A real directory here is operator-managed — leave it, use it. + return pluginDir; + } + symlinkSync(skillsSrc, link, "dir"); + return pluginDir; + } catch (e) { + console.warn( + `[packs] could not materialize the skill plugin for registry "${registry}": ${e instanceof Error ? e.message : String(e)}`, + ); + return undefined; + } + } + #save(): void { this.#persist?.({ defaultPack: this.#defaultPack, packs: this.#packs, registries: this.#registries }); } diff --git a/src/daemon/pipeline/pack.test.ts b/src/daemon/pipeline/pack.test.ts index 157a65c7..dce30c38 100644 --- a/src/daemon/pipeline/pack.test.ts +++ b/src/daemon/pipeline/pack.test.ts @@ -117,15 +117,50 @@ describe("loadPack", () => { expect(pack.pipeline[1].onFail).toEqual({ action: "retry", max: 3 }); }); - test("register() installs the pack's skills + gates into the registries", () => { + test("register() installs the pack's skills + gates into the registries, scoped by pack id", () => { const mgr = new PipelineManager(new PipelineStore(new Database(":memory:"))); mgr.installPack(loadPack(fullPack())); - expect(mgr.registries.skills.has("spec")).toBe(true); - expect(mgr.registries.skills.has("build")).toBe(true); - expect(mgr.registries.gates.has("tests_pass")).toBe(true); + // Registered under `/` (scoped.ts) — never the bare id, so two + // packs declaring the same skill/gate id can't overwrite each other. + expect(mgr.registries.skills.has("aif-test/spec")).toBe(true); + expect(mgr.registries.skills.has("aif-test/build")).toBe(true); + expect(mgr.registries.gates.has("aif-test/tests_pass")).toBe(true); + expect(mgr.registries.skills.has("spec")).toBe(false); + expect(mgr.registries.gates.has("tests_pass")).toBe(false); expect(mgr.registries.packs.has("aif-test")).toBe(true); }); + test("two packs declaring the same skill + gate ids coexist (no last-wins overwrite)", () => { + const mgr = new PipelineManager(new PipelineStore(new Database(":memory:"))); + const twin = writePack(`schema: codeoid/pack@v1 +id: twin +name: Twin +version: 0.1.0 +skills: + - { id: spec, kind: prompt, template: "twin-spec" } +gates: + - { id: tests_pass, kind: command, run: "false" } +phases: + - { id: one, skill: spec, gate: tests_pass } +`); + mgr.installPack(loadPack(fullPack())); + mgr.installPack(loadPack(twin)); + expect(mgr.registries.skills.resolve("aif-test/spec")).toBeDefined(); + expect(mgr.registries.skills.resolve("twin/spec")).toMatchObject({ kind: "prompt", template: "twin-spec" }); + expect(mgr.registries.gates.has("aif-test/tests_pass")).toBe(true); + expect(mgr.registries.gates.has("twin/tests_pass")).toBe(true); + // A run created from either pack validates against ITS OWN entries. + const run = mgr.create({ + name: "t", + pack: "twin", + accountId: "a", + projectId: "p", + createdBy: "u", + }); + expect(run.packId).toBe("twin"); + expect(run.phases[0]!.def).toMatchObject({ skill: "spec", gate: "tests_pass" }); + }); + test("kind defaults to 'skill' when a phase declares only a skill", () => { const dir = writePack(`schema: codeoid/pack@v1 id: p @@ -415,7 +450,7 @@ describe("pack loader — probe gates", () => { const pack = loadPack(writePack(PROBE_MANIFEST)); const r = createRegistries(); pack.register(r); - const gate = r.gates.resolve("spec-done"); + const gate = r.gates.resolve("probe-pack/spec-done"); // pack-scoped id (scoped.ts) expect(gate).toBeDefined(); const workdir = mkdtempSync(join(tmpdir(), "probe-wd-")); @@ -454,7 +489,7 @@ phases: const workdir = mkdtempSync(join(tmpdir(), "probe-wd-")); dirs.push(workdir); writeFileSync(join(workdir, "go.mod"), "module x\n"); - const v = await r.gates.resolve("impl-verify")!.evaluate({ + const v = await r.gates.resolve("probe-pack/impl-verify")!.evaluate({ pipeline: { workdir } as never, phase: { id: "implement", kind: "skill" }, }); diff --git a/src/daemon/pipeline/pack.ts b/src/daemon/pipeline/pack.ts index 909ad00b..801d9992 100644 --- a/src/daemon/pipeline/pack.ts +++ b/src/daemon/pipeline/pack.ts @@ -19,6 +19,7 @@ import type { PipelineRegistries, SkillPlugin, } from "./interface"; +import { scopedId } from "./scoped"; // ── Manifest schema (the pack.yaml contract) ────────────────────────────── @@ -301,9 +302,13 @@ export function loadPack(dir: string, opts: LoadPackOptions = {}): LoadedPack { dir, gateSpecs: m.gates.map((g) => ({ id: g.id, kind: g.kind })), pipeline, + // Registered under `/` (scoped.ts) so two installed packs that + // both declare `review` / `tests_pass` coexist instead of overwriting each + // other in the daemon-wide registries. Phase defs keep the bare ids; the + // engine / skill kind / create-validation resolve them pack-first. register(r: PipelineRegistries): void { - for (const s of skills) r.skills.register(s); - for (const g of gates) r.gates.register(g); + for (const s of skills) r.skills.register({ ...s, id: scopedId(m.id, s.id) }); + for (const g of gates) r.gates.register({ ...g, id: scopedId(m.id, g.id) }); }, }; } diff --git a/src/daemon/pipeline/scoped.ts b/src/daemon/pipeline/scoped.ts new file mode 100644 index 00000000..0378874a --- /dev/null +++ b/src/daemon/pipeline/scoped.ts @@ -0,0 +1,37 @@ +/** + * Pack-scoped registry ids. + * + * A pack registers its skills and gates under `/` so two packs + * declaring the same bare id coexist instead of overwriting each other + * last-wins in the daemon-wide registries (org-dev and yash-dev both declare + * `review`, `ship`, and `tests_pass`; before this the second install silently + * replaced the first's, and a run from the first pack drove the wrong skill). + * + * Phase defs keep the bare id the author wrote — display, CLI, and the web + * PackBrowser are unchanged. Resolution scopes the id by the run's `packId` + * first and falls back to the bare id, which is how built-in gates (`always`, + * `manual`) and explicit-`phases` plans (skills registered directly, no pack) + * keep resolving exactly as before. + */ + +import type { Registry } from "./interface"; + +/** `/`, or the bare id when there is no pack. */ +export const scopedId = (packId: string | undefined, id: string): string => (packId ? `${packId}/${id}` : id); + +/** Resolve `id` for a run: the pack-scoped entry when the run has a pack and the + * pack declared it, else the bare (built-in / directly registered) entry. */ +export function resolveScoped( + reg: Registry, + packId: string | undefined, + id: string, +): T | undefined { + if (packId) { + const scoped = reg.resolve(scopedId(packId, id)); + if (scoped) return scoped; + } + return reg.resolve(id); +} + +export const hasScoped = (reg: Registry, packId: string | undefined, id: string): boolean => + resolveScoped(reg, packId, id) !== undefined; diff --git a/src/daemon/pipeline/skill-kind.test.ts b/src/daemon/pipeline/skill-kind.test.ts index ac4145db..7eafdb76 100644 --- a/src/daemon/pipeline/skill-kind.test.ts +++ b/src/daemon/pipeline/skill-kind.test.ts @@ -4,12 +4,13 @@ import { createRegistries } from "./registry"; import type { PhaseRunner, PhaseRunRequest } from "./runner"; import { makeSkillPhaseKind } from "./skill-kind"; -function ctxFor(phase: PhaseDef, skills: SkillPlugin[]): PhaseCtx { +function ctxFor(phase: PhaseDef, skills: SkillPlugin[], packId?: string): PhaseCtx { const registries = createRegistries(); for (const s of skills) registries.skills.register(s); const pipeline: PipelineState = { id: "p", name: "p", + ...(packId ? { packId } : {}), phases: [{ def: phase, state: { status: "running", startedAt: 1, attempts: 0 } }], cursor: 0, status: "running", @@ -23,6 +24,30 @@ function ctxFor(phase: PhaseDef, skills: SkillPlugin[]): PhaseCtx { } describe("skill phase kind", () => { + test("resolves the run's pack-scoped skill before a bare one of the same id", async () => { + const bare: SkillPlugin = { + id: "hello", + kind: "fn", + async run() { + return { summary: "bare" }; + }, + }; + const scoped: SkillPlugin = { + id: "mypack/hello", + kind: "fn", + async run() { + return { summary: "scoped" }; + }, + }; + const kind = makeSkillPhaseKind(); + const phase: PhaseDef = { id: "one", kind: "skill", skill: "hello" }; + // With a packId the pack's own entry wins; without one the bare entry resolves. + expect(await kind.run(ctxFor(phase, [bare, scoped], "mypack"))).toMatchObject({ summary: "scoped" }); + expect(await kind.run(ctxFor(phase, [bare, scoped]))).toMatchObject({ summary: "bare" }); + // A pack run whose pack didn't declare the id still falls back to bare. + expect(await kind.run(ctxFor(phase, [bare], "mypack"))).toMatchObject({ summary: "bare" }); + }); + test("runs an fn skill natively and passes with its summary", async () => { const skill: SkillPlugin = { id: "hello", diff --git a/src/daemon/pipeline/skill-kind.ts b/src/daemon/pipeline/skill-kind.ts index 44b9c40c..ee1d03dd 100644 --- a/src/daemon/pipeline/skill-kind.ts +++ b/src/daemon/pipeline/skill-kind.ts @@ -8,6 +8,7 @@ import type { PhaseCtx, PhaseKind, PhaseRunResult, PipelinePhase, SkillPlugin } from "./interface"; import type { PhaseRunner } from "./runner"; +import { resolveScoped } from "./scoped"; /** Compose a phase's prompt: the skill command/template, the run's goal, and — * on a revise re-run — the phase's prior output + the accumulated human @@ -42,7 +43,8 @@ export function makeSkillPhaseKind(runner?: PhaseRunner): PhaseKind { if (!skillId) { return { outcome: "failed", reason: `phase "${ctx.phase.id}" has kind:"skill" but no skill id` }; } - const skill = ctx.registries.skills.resolve(skillId); + // The run's own pack entry first (`/`), bare second — scoped.ts. + const skill = resolveScoped(ctx.registries.skills, ctx.pipeline.packId, skillId); if (!skill) return { outcome: "failed", reason: `unknown skill "${skillId}"` }; return runSkill(skill, ctx, runner); }, diff --git a/src/daemon/providers/claude/index.ts b/src/daemon/providers/claude/index.ts index 35b92326..e20fa29a 100644 --- a/src/daemon/providers/claude/index.ts +++ b/src/daemon/providers/claude/index.ts @@ -78,6 +78,21 @@ function packAgentsOption( return { agents }; } +/** + * Session-scoped skill plugins (docs/pack-loading.md §3a) → the SDK's `plugins` + * option. Each dir is a Claude-Code-plugin-shaped tree (`.claude-plugin/ + * plugin.json` + `skills/`) that PackService synthesizes for a trusted pack's + * registry under `pipeline.skillScope: "session"`. Loading it here — instead of + * symlinking into `~/.claude/skills` — is what keeps a methodology's skills + * inside codeoid sessions and out of every other Claude Code session on the + * machine. Plugin skills resolve both bare (`/spec`) and namespaced + * (`/:spec`), so pack `command:` values need no rewrite. + */ +export function packPluginsOption(dirs?: readonly string[]): { plugins?: { type: "local"; path: string }[] } { + if (!dirs || dirs.length === 0) return {}; + return { plugins: dirs.map((path) => ({ type: "local" as const, path })) }; +} + // ── Initialisation options ──────────────────────────────────────────────────── export interface ClaudeProviderInit { @@ -159,6 +174,10 @@ export class ClaudeProvider implements SessionProvider { * set forces a rebuild (like #builtSystemPromptAppend) so a just-approved * command is actually in allowedTools on the retry (#233). */ #builtSkillAllowRules = ""; + /** Session-scoped skill plugin dirs the LIVE query loop was built with. The + * SDK fixes `plugins` at query construction, and a pipeline run swaps the pack + * activation between phases, so a changed set forces a rebuild too. */ + #builtPluginDirs = ""; /** Last TurnOpts — the retry after a skill approval rebuilds the loop with * these, then re-pushes #lastPushedContent into the SAME turn queue (#233). */ #lastTurnOpts: TurnOpts | null = null; @@ -434,6 +453,7 @@ export class ClaudeProvider implements SessionProvider { const desiredAppend = opts.systemPromptAppend ?? ""; const skillAllowRules = this.#resolveSkillGrants(opts); const desiredGrants = skillAllowRules.join("\n"); + const desiredPlugins = (opts.pluginDirs ?? []).join("\n"); // Exact tool names we hand the SDK as pre-approved. Kept as its own list // (rather than inlined into `allowedTools`) because the PreToolUse hook has @@ -451,21 +471,25 @@ export class ClaudeProvider implements SessionProvider { if (this.#consumerTask && this.#inputQueue && !this.#inputQueue.closed) { if ( this.#builtSystemPromptAppend === desiredAppend && - this.#builtSkillAllowRules === desiredGrants + this.#builtSkillAllowRules === desiredGrants && + this.#builtPluginDirs === desiredPlugins ) { return; } // Per-turn contributions the SDK fixes at query construction changed, so // the warm loop would silently use the stale value — rebuild instead. - // Two triggers: the system-prompt append (before_turn hooks, memory - // workspace-index refresh — #153) and the skill-command grants (a just- - // approved command must reach allowedTools on the retry — #233). The + // Three triggers: the system-prompt append (before_turn hooks, memory + // workspace-index refresh — #153), the skill-command grants (a just- + // approved command must reach allowedTools on the retry — #233), and the + // session-scoped skill plugins (a pack activation applied or swapped). The // fresh query RESUMES the same backing session, so no context is lost. // console.log, not error: an expected control-flow event. const reason = this.#builtSystemPromptAppend !== desiredAppend ? `systemPromptAppend changed (${this.#builtSystemPromptAppend.length}B → ${desiredAppend.length}B)` - : "skill-command grants changed"; + : this.#builtSkillAllowRules !== desiredGrants + ? "skill-command grants changed" + : "session-scoped skill plugins changed"; console.log( `[claude-provider ${this.#init.sessionId.slice(0, 8)}] ${reason} — rebuilding query loop`, ); @@ -481,6 +505,7 @@ export class ClaudeProvider implements SessionProvider { this.#inputQueue = new AsyncQueue(); this.#builtSystemPromptAppend = desiredAppend; this.#builtSkillAllowRules = desiredGrants; + this.#builtPluginDirs = desiredPlugins; this.#loopGeneration += 1; const myGeneration = this.#loopGeneration; @@ -591,23 +616,31 @@ export class ClaudeProvider implements SessionProvider { // (Pack subagents live in the registry cache, not a `.claude/agents` // tier, so they're injected rather than discovered.) ...packAgentsOption(opts.subagents), + // Session-scoped pack skills (`pipeline.skillScope: "session"`) → the + // SDK's `plugins` option: discovered for THIS session only, never + // linked into `~/.claude/skills`. + ...packPluginsOption(opts.pluginDirs), ...sessionOpts, // Load BOTH the project tier (`/.claude`) and the user tier - // (`~/.claude`). A pack installs its runnable skills into - // `~/.claude/skills/` (pack-service #linkSkills), so a project-only - // source silently dropped them and `/spec`-style skill invocations came - // back "Unknown command". `skills: "all"` then enables every discovered - // skill for auto-selection + slash invocation. In the sandbox `~/.claude` - // is codeoid-controlled; in local mode this also surfaces the user's own - // skills, which is the intended behaviour. + // (`~/.claude`). Under the default global skill scope a pack installs + // its runnable skills into `~/.claude/skills/` (pack-service + // #linkSkills), so a project-only source silently dropped them and + // `/spec`-style skill invocations came back "Unknown command". + // `skills: "all"` then enables every discovered skill (user tier, + // project tier, and session plugins alike) for auto-selection + slash + // invocation. In the sandbox `~/.claude` is codeoid-controlled; in local + // mode this also surfaces the user's own skills, which is the intended + // behaviour. settingSources: ["project", "user"], skills: "all", // Permission to RUN a skill's command is not permission to READ the // files it touches — the two gates are independent and fail in that - // order. See skillSandboxDirs. + // order. See skillSandboxDirs. Session plugins' `skills/` are symlinks + // into the registry cache, so they need the same real-parent grant. additionalDirectories: skillSandboxDirs([ join(homedir(), ".claude", "skills"), join(opts.workdir, ".claude", "skills"), + ...pluginSkillDirs(opts.pluginDirs), ]), hooks: { @@ -860,6 +893,10 @@ export class ClaudeProvider implements SessionProvider { const declared = skillCommandAllowRules([ join(homedir(), ".claude", "skills"), join(opts.workdir, ".claude", "skills"), + // Session-scoped pack skills declare `!`…`` substitutions too; without + // scanning them a plugin skill would expand to nothing exactly like a + // linked one did before #233. + ...pluginSkillDirs(opts.pluginDirs), ]); if (declared.length === 0) return []; const decided = this.#init.store.getSkillCommandGrants(this.#init.workspaceId); @@ -1335,6 +1372,13 @@ export function translateSDKMessage( // ── Helpers ─────────────────────────────────────────────────────────────────── +/** The `skills/` tier of each session-scoped plugin dir — the extra roots that + * skillSandboxDirs / skillCommandAllowRules must scan alongside the user and + * project tiers when a pack is activated under `skillScope: "session"`. */ +export function pluginSkillDirs(pluginDirs?: readonly string[]): string[] { + return (pluginDirs ?? []).map((d) => join(d, "skills")); +} + /** * Absolute directories the agent must be able to READ for skills to work, * beyond `cwd`. diff --git a/src/daemon/providers/interface.ts b/src/daemon/providers/interface.ts index 394d2f6f..817ff09b 100644 --- a/src/daemon/providers/interface.ts +++ b/src/daemon/providers/interface.ts @@ -126,6 +126,15 @@ export interface TurnOpts { * user settings tier). Absent = none. */ subagents?: readonly PackSubagent[]; + /** + * Session-scoped skill plugins from an ambient-activated pack + * (`pipeline.skillScope: "session"`, docs/pack-loading.md §3a). Each entry is + * a Claude-Code-plugin-shaped directory (`.claude-plugin/plugin.json` + + * `skills/`); the Claude backend hands them to the SDK `plugins` option so the + * pack's slash skills exist for THIS session without touching + * `~/.claude/skills`. Other backends currently ignore them. Absent = none. + */ + pluginDirs?: readonly string[]; } // ── Normalized turn result ──────────────────────────────────────────────────── diff --git a/src/daemon/session-manager.ts b/src/daemon/session-manager.ts index 3ec3468e..888ee60c 100644 --- a/src/daemon/session-manager.ts +++ b/src/daemon/session-manager.ts @@ -476,6 +476,9 @@ export class SessionManager { // The operator's model maps, for the pre-flight `pack show --resolve` // view (docs/role-model-binding.md §4) — same maps pipeline.create reads. modelConfig: { modelTiers: p?.modelTiers, modelRoles: p?.modelRoles }, + // Machine-wide symlinks (global) vs per-session SDK plugins (session) for + // a trusted pack's registry skills (docs/pack-loading.md §3a). + skillScope: p?.skillScope, }); } diff --git a/src/daemon/session.ts b/src/daemon/session.ts index 9fc8bda0..55865c0e 100644 --- a/src/daemon/session.ts +++ b/src/daemon/session.ts @@ -1906,6 +1906,7 @@ export class Session { sender: recoverySender, mode: this.#mode, subagents: this.#pack?.subagents, + pluginDirs: this.#packPluginDirs(), }); this.#activeRun = recoveryRun; this.#eventConsumerTask = this.#consumeEvents(recoveryRun, recoverySender); @@ -1938,6 +1939,7 @@ export class Session { sender, mode: this.#mode, subagents: this.#pack?.subagents, + pluginDirs: this.#packPluginDirs(), }); this.#activeRun = run; this.#eventConsumerTask = this.#consumeEvents(run, sender); @@ -3200,6 +3202,15 @@ export class Session { this.#pack = pack; } + /** Session-scoped skill plugins from the active pack (docs/pack-loading.md + * §3a) — read per turn, like subagents, so a phase swap takes effect on the + * next turn. Undefined (not `[]`) when there are none, so the provider's + * rebuild guard sees "no plugins" as one stable value. */ + #packPluginDirs(): readonly string[] | undefined { + const dir = this.#pack?.skillsPluginDir; + return dir ? [dir] : undefined; + } + #buildPromptAppend(): string | undefined { const parts: string[] = []; if (this.role === "conductor") parts.push(CONDUCTOR_SYSTEM_PROMPT_APPEND); diff --git a/src/tests/provider-claude.test.ts b/src/tests/provider-claude.test.ts index a0e8e667..03abfb93 100644 --- a/src/tests/provider-claude.test.ts +++ b/src/tests/provider-claude.test.ts @@ -81,6 +81,8 @@ import { buildAgentEnv, skillCommandAllowRules, skillSandboxDirs, + packPluginsOption, + pluginSkillDirs, } from "../daemon/providers/claude/index.js"; import { mkdtempSync, mkdirSync, writeFileSync, rmSync, symlinkSync, realpathSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -393,6 +395,32 @@ describe("skillSandboxDirs", () => { }); }); +describe("packPluginsOption / pluginSkillDirs (session-scoped pack skills)", () => { + it("maps plugin dirs to the SDK `plugins` option, and omits the key when there are none", () => { + expect(packPluginsOption(undefined)).toEqual({}); + expect(packPluginsOption([])).toEqual({}); + expect(packPluginsOption(["/p/ai-factory"])).toEqual({ plugins: [{ type: "local", path: "/p/ai-factory" }] }); + }); + + it("derives each plugin's skills tier for the sandbox + grant scans", () => { + expect(pluginSkillDirs(undefined)).toEqual([]); + expect(pluginSkillDirs(["/p/a", "/p/b"])).toEqual([join("/p/a", "skills"), join("/p/b", "skills")]); + }); + + it("grants the real parent of a plugin's symlinked skills tier (same rule as linked skills)", () => { + const tmp = mkdtempSync(join(tmpdir(), "codeoid-plugin-")); + const cacheSkills = join(tmp, "packs", "reg", "skills"); + mkdirSync(join(cacheSkills, "spec"), { recursive: true }); + mkdirSync(join(cacheSkills, "templates"), { recursive: true }); + const plugin = join(tmp, "plugins", "reg"); + mkdirSync(plugin, { recursive: true }); + symlinkSync(cacheSkills, join(plugin, "skills")); + const dirs = skillSandboxDirs(pluginSkillDirs([plugin])); + expect(dirs).toContain(realpathSync(cacheSkills)); // holds templates/ + rmSync(tmp, { recursive: true, force: true }); + }); +}); + describe("skillCommandAllowRules", () => { const write = (dir: string, name: string, body: string) => { mkdirSync(join(dir, name), { recursive: true }); From 92c0f5ab9e1db10ab668cf1f54b86d54e40080a8 Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Mon, 14 Sep 2026 18:52:06 +0800 Subject: [PATCH 2/2] fix: harden session-scoped skills after an adversarial audit of #336 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit of the first commit (an independent review pass plus a trace of every registry consumer and activation site) found one hole and two gaps; all three are closed here, with tests. **The plugin layout dropped the symlink guard the global path has.** `#ensureSkillPlugin` linked the WHOLE `/skills` directory into the plugin, while `#linkSkills` deliberately refuses a `skills/` that is itself a symlink. Reproduced: a registry shipping `skills/evil -> /some/dir` was discovered as a skill, and — because `skillSandboxDirs` widens the read sandbox to the real parent of every entry — its target's parent landed in `additionalDirectories`, and its `!`…`` substitutions in `allowedTools`. The session scope was strictly weaker than the global one. The plugin's `skills/` is now a real directory holding one symlink per real skill directory, under the same lstat guard; stale links (a skill removed on `registry refresh`, or one pointing outside the cache) are pruned on the next activation, a whole-dir link from the pre-release layout is replaced, and a real directory an operator placed there is left alone. **Collaboration adoption lost the plugin.** The compiled goal pack was built from `adoption.subagents` but not `adoption.skillsPluginDir`, and `roleChildPosture` never carried it, so under `session` scope a `--pack` collaboration's orchestrator and role-children had none of the pack's slash skills while a global-scope adoption saw them all. Both now carry it (children on spawn; the resume path already re-derives no adoption, unchanged here). **Explicit-`phases` plans could borrow a pack's entries by bare id.** Packs no longer register bare ids, so an explicit plan naming `spec` or `tests_pass` now fails at create — that borrowing was the same cross-pack leakage from the other side, and the web UI and CLI always send `pack`. Made deliberate rather than accidental: the create error lists the qualified ids that exist (`"org-dev/spec"`), which an explicit plan can name directly; CHANGELOG and docs say so. Also: `CODEOID_PIPELINE_SKILL_SCOPE` env override for sandbox/CI images; `pipeline.pack.list` reports the daemon's live `skillScope` and `codeoid pack list` prints it; docs record the bare-name collision rule (a user- or project-tier skill wins bare `/spec`, verified against the binary; `/:spec` names the pack's). Co-Authored-By: Claude Fable 5.1 Signed-off-by: Yash Datta --- CHANGELOG.md | 32 ++++++--- docs/pack-loading.md | 28 ++++++-- packages/protocol/src/types.ts | 5 ++ src/config.ts | 3 + src/daemon/collaboration.ts | 7 +- src/daemon/pipeline/manager.ts | 22 ++++-- src/daemon/pipeline/pack-service.test.ts | 33 +++++++++ src/daemon/pipeline/pack-service.ts | 91 ++++++++++++++++++------ src/daemon/pipeline/pack.test.ts | 22 ++++++ src/daemon/pipeline/scoped.ts | 10 +++ src/daemon/session-manager.ts | 13 +++- src/terminal/pack-format.ts | 9 +++ src/tests/collaboration.test.ts | 14 ++++ src/tests/config.test.ts | 21 ++++++ 14 files changed, 264 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 594534dd..f3cd62d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,15 +32,19 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). something else (an org bundle linked by hand, another toolkit) that is the wrong scope. With `skillScope: "session"` nothing is linked; codeoid synthesizes a Claude-Code-plugin-shaped directory per registry - (`~/.codeoid/plugins//`: a manifest plus `skills → /skills`) - and hands it to the SDK's `plugins` option for pack-activated sessions and - pipeline phases, so the methodology's skills exist inside codeoid runs and - nowhere else. Plugin skills resolve both bare (`/spec`) and namespaced - (`/:spec`), so pack `command:` values are unchanged; the same trust - rule applies (an untrusted pack contributes no runnable skills either way); - the read sandbox and the skill-command grants scan the plugin tier too. The - default stays `global`. Non-Claude backends ignore the plugin dirs, as they - already ignore pack subagents. (docs/pack-loading.md §3a) + (`~/.codeoid/plugins//`: a manifest plus one symlink per real skill + directory in the registry cache, under the same lstat guard as global + linking) and hands it to the SDK's `plugins` option for pack-activated + sessions and pipeline phases, so the methodology's skills exist inside + codeoid runs and nowhere else. Plugin skills resolve both bare (`/spec`) and + namespaced (`/:spec`), so pack `command:` values are unchanged; on a + bare-name collision the user- or project-tier skill wins, as it does for a + global link. The same trust rule applies (an untrusted pack contributes no + runnable skills either way); the read sandbox and the skill-command grants + scan the plugin tier too. The default stays `global`; + `CODEOID_PIPELINE_SKILL_SCOPE` sets it per invocation. Non-Claude backends + ignore the plugin dirs, as they already ignore pack subagents. + (docs/pack-loading.md §3a) ### Fixed @@ -52,9 +56,17 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). second pack's skill. Packs now register under `/`; the engine, the skill phase kind, and create-time validation resolve a run's own pack entry first and fall back to the bare id, so built-in gates (`always`, `manual`) and - explicit-`phases` plans keep resolving exactly as before. Phase defs, CLI + directly registered entries keep resolving exactly as before. Phase defs, CLI output, and the web Pack Browser still show the ids as authored. + One deliberate consequence: an explicit-`phases` plan (wire `pipeline.create` + with `phases`, not `pack`) can no longer borrow an installed pack's skill or + gate by its bare id — that borrowing was the same leakage, just from the + other side. Name the entry by its qualified id (`skill: "org-dev/spec"`), or + create the run with `pack`; the create error now lists the qualified ids that + exist. The web UI and CLI always send `pack`, so only direct API/SDK clients + are affected, and `pipeline.pack.list` now reports the daemon's `skillScope`. + - **A dangling skill symlink blocked that skill from ever being linked again.** An older loader linked registry skills from a temp clone under `/tmp`; after the cache moved, those links pointed at nothing. `existsSync` is false for a diff --git a/docs/pack-loading.md b/docs/pack-loading.md index cdab5d6d..7406a4a7 100644 --- a/docs/pack-loading.md +++ b/docs/pack-loading.md @@ -64,13 +64,22 @@ A trusted pack's registry `skills/` can reach a session two ways, chosen by | `skillScope` | How the skills reach a session | Who else sees them | | --- | --- | --- | | `global` (default) | `#linkSkills` symlinks each `skills/` into `~/.claude/skills` on install / trust / refresh (additive; a **dangling** link left by an older loader is repaired, a real dir or live link is never touched) | every Claude Code session on the machine — the user's own same-named skills win collisions | -| `session` | nothing is linked; `resolveActivation()` returns `skillsPluginDir`, a synthesized Claude-Code-plugin dir (`~/.codeoid/plugins//` = `.claude-plugin/plugin.json` + `skills → /skills`) that the Claude backend passes to the SDK `plugins` option for that session's turns | only pack-activated codeoid sessions and pipeline phases | +| `session` | nothing is linked; `resolveActivation()` returns `skillsPluginDir`, a synthesized Claude-Code-plugin dir (`~/.codeoid/plugins//` = `.claude-plugin/plugin.json` + a real `skills/` holding one symlink per real skill directory in `/skills`) that the Claude backend passes to the SDK `plugins` option for that session's turns | only pack-activated codeoid sessions and pipeline phases | Both scopes apply the same trust rule (an untrusted pack contributes no -runnable skills either way), and both scan the skills for their `!`…`` -substitutions so the command grants (#233) and the read sandbox work -identically. Plugin skills resolve both bare (`/spec`) and namespaced -(`/:spec`), so pack `command:` values are unchanged. +runnable skills either way) and the same lstat guard (a `skills/` that +is itself a symlink in the registry is never propagated — under session scope +a whole-dir link would have exposed it *and* widened the read sandbox to its +real parent), and both scan the skills for their `!`…`` substitutions so the +command grants (#233) and the read sandbox work identically. Plugin skills +resolve both bare (`/spec`) and namespaced (`/:spec`), so pack +`command:` values are unchanged. On a bare-name collision the user- or +project-tier skill wins under both scopes (verified against the binary); a +pack that wants the registry's version regardless names it +`/:`. Stale plugin links (a skill removed on `registry +refresh`) are pruned on the next activation; a real directory an operator +places in the plugin's `skills/` is left alone. `CODEOID_PIPELINE_SKILL_SCOPE` +sets the scope per invocation. `session` is the scope for a machine whose `~/.claude` is owned by something else (an org bundle symlinked by hand, another toolkit): the methodology's @@ -82,8 +91,13 @@ they ignore pack subagents. Registry skills and gates are registered under `/` (`scoped.ts`) so two installed packs declaring the same bare id (`review`, `ship`, -`tests_pass`) coexist; a run resolves its own pack's entry first and falls back -to the bare id for built-in gates and explicit-`phases` plans. +`tests_pass`) coexist; a run created from a pack resolves its own pack's entry +first and falls back to the bare id for built-in gates (`always`, `manual`). +An explicit-`phases` plan has no pack to scope by: it resolves built-ins and +directly registered entries by bare id, and an installed pack's entries by +their qualified id (`skill: "org-dev/spec"`). It can no longer borrow a pack's +entry by bare id — the create error lists the qualified ids that exist. +`pipeline.pack.list` reports the daemon's live `skillScope`. Persistence goes through one shared config mutator (`mutateConfigFile`) that read → mutates → validates against `RootSchema` → atomically writes `0o600` — diff --git a/packages/protocol/src/types.ts b/packages/protocol/src/types.ts index cb5f3128..1f5f0382 100644 --- a/packages/protocol/src/types.ts +++ b/packages/protocol/src/types.ts @@ -2464,6 +2464,11 @@ export interface PackListResultMsg { installed: PackWire[]; available: AvailablePackWire[]; registries: RegistryWire[]; + /** How a trusted pack's registry skills reach sessions on this daemon + * (`config.pipeline.skillScope`): `global` = symlinked into `~/.claude/skills` + * machine-wide; `session` = exposed only inside pack-activated sessions as an + * SDK plugin. Optional (additive) — absent from older daemons. */ + skillScope?: "global" | "session"; } // ============================================================================= diff --git a/src/config.ts b/src/config.ts index d390790d..1e27ab66 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1219,6 +1219,9 @@ const ENV_OVERRIDES: readonly EnvOverride[] = [ // without touching config.json (on by default; set false to opt out). Other // pipeline knobs are file-config only, matching the dispatch/conductor convention. { env: "CODEOID_PIPELINE_ENABLED", path: "pipeline.enabled", kind: "boolean" }, + // Skill scope (global | session) — per-invocation, e.g. a sandbox image that + // must never write into ~/.claude/skills. The schema enum rejects other values. + { env: "CODEOID_PIPELINE_SKILL_SCOPE", path: "pipeline.skillScope", kind: "string" }, { env: "CODEOID_FALLBACK_MODEL", path: "session.fallbackModel", kind: "string" }, // Hooks kill switch — disable every configured hook per-invocation without // touching config.json. Entries themselves are file-config only. diff --git a/src/daemon/collaboration.ts b/src/daemon/collaboration.ts index 12426e15..fbe11b92 100644 --- a/src/daemon/collaboration.ts +++ b/src/daemon/collaboration.ts @@ -496,7 +496,7 @@ export function roleChildPosture( child: PlannedChild, parentSessionId: string, constitution: string, - adopted?: { packId: string; role: RoleDef }, + adopted?: { packId: string; role: RoleDef; skillsPluginDir?: string }, ): { role: "worker"; workerShape: "ship" | "scout"; @@ -512,6 +512,10 @@ export function roleChildPosture( }; roleName: string; subagents: never[]; + /** Session-scoped pack skills (`pipeline.skillScope: "session"`), carried + * from the adopted pack so a child can run the methodology's slash skills + * exactly as it could under a global-scope link. */ + skillsPluginDir?: string; }; collaborationRole: { parentSessionId: string; @@ -555,6 +559,7 @@ export function roleChildPosture( }, roleName: child.roleName, subagents: [], + ...(adopted?.skillsPluginDir ? { skillsPluginDir: adopted.skillsPluginDir } : {}), }, collaborationRole: { parentSessionId, diff --git a/src/daemon/pipeline/manager.ts b/src/daemon/pipeline/manager.ts index 61e7808d..a4bb45f5 100644 --- a/src/daemon/pipeline/manager.ts +++ b/src/daemon/pipeline/manager.ts @@ -18,11 +18,11 @@ import { resolveModelIdForProvider } from "../models.js"; import { type ModelBinding, type ModelBindingConfig, resolveBinding } from "./binding"; import { registerBuiltins } from "./builtin"; import { PipelineEngine } from "./engine"; -import type { Pack, PhaseDef, PipelineRegistries, PipelineState } from "./interface"; +import type { Pack, PhaseDef, PipelineRegistries, PipelineState, Registry } from "./interface"; import { isTerminal } from "./interface"; import { createRegistries } from "./registry"; import type { PhaseRunner } from "./runner"; -import { hasScoped } from "./scoped"; +import { hasScoped, scopedMatches } from "./scoped"; import { makeSkillPhaseKind } from "./skill-kind"; import type { PipelineStore } from "./store"; @@ -429,10 +429,20 @@ export class PipelineManager { } /** `packId` scopes gate/skill lookups to the pack the plan came from - * (`/` first, bare second — scoped.ts); absent for explicit plans. */ + * (`/` first, bare second — scoped.ts); absent for explicit + * plans, which resolve built-ins and directly registered entries by bare id + * and an installed pack's entries by their qualified `/`. An + * explicit plan naming a pack's entry by bare id is told the qualified ids + * that exist rather than a flat "unknown". */ #validate(phases: PhaseDef[], packId?: string): void { if (phases.length === 0) throw new Error("pipeline must declare at least one phase"); const seen = new Set(); + const unknown = (what: string, reg: Registry, id: string): string => { + const hint = packId ? [] : scopedMatches(reg, id); + return hint.length > 0 + ? `unknown ${what} "${id}" — installed packs declare it as ${hint.map((h) => `"${h}"`).join(", ")}; name it that way, or create the run with \`pack\`` + : `unknown ${what} "${id}"`; + }; for (const p of phases) { if (seen.has(p.id)) throw new Error(`duplicate phase id "${p.id}"`); seen.add(p.id); @@ -440,15 +450,15 @@ export class PipelineManager { throw new Error(`phase "${p.id}": unknown kind "${p.kind}"`); } if (p.gate && !hasScoped(this.#registries.gates, packId, p.gate)) { - throw new Error(`phase "${p.id}": unknown gate "${p.gate}"`); + throw new Error(`phase "${p.id}": ${unknown("gate", this.#registries.gates, p.gate)}`); } if (p.entryGate && !hasScoped(this.#registries.gates, packId, p.entryGate)) { - throw new Error(`phase "${p.id}": unknown entry gate "${p.entryGate}"`); + throw new Error(`phase "${p.id}": ${unknown("entry gate", this.#registries.gates, p.entryGate)}`); } if (p.kind === "skill") { if (!p.skill) throw new Error(`phase "${p.id}": kind "skill" requires a skill id`); if (!hasScoped(this.#registries.skills, packId, p.skill)) { - throw new Error(`phase "${p.id}": unknown skill "${p.skill}"`); + throw new Error(`phase "${p.id}": ${unknown("skill", this.#registries.skills, p.skill)}`); } } } diff --git a/src/daemon/pipeline/pack-service.test.ts b/src/daemon/pipeline/pack-service.test.ts index 32596d3d..f1422c8a 100644 --- a/src/daemon/pipeline/pack-service.test.ts +++ b/src/daemon/pipeline/pack-service.test.ts @@ -357,6 +357,39 @@ describe("install / trust / select / remove", () => { expect(svc.resolveActivation("p").skillsPluginDir).toBe(join(pluginsDir, "reg")); }); + test("skillScope: session — the plugin never exposes a symlinked registry entry, and prunes stale links", async () => { + const fixture = tmp(); + writeRegistry(fixture, ["p"], ["spec"]); + // Hostile entry inside the registry's skills/: a whole-dir plugin link would + // expose it AND widen the read sandbox to its real parent (skillSandboxDirs). + symlinkSync("/etc", join(fixture, "skills", "evil"), "dir"); + const cacheDir = join(tmp(), "c"); + const pluginsDir = join(tmp(), "plugins"); + const { svc } = makeService({ cacheDir, pluginsDir, skillScope: "session", fixture }); + await svc.addRegistry({ url: "https://github.com/a/reg.git" }); + svc.install({ packId: "p", trusted: true }); + // The plugin is materialized lazily, at activation (not at install), so it + // self-heals on every session/phase that uses it. + svc.resolveActivation("p"); + const pluginSkills = join(pluginsDir, "reg", "skills"); + const fs = require("node:fs"); + // A real directory of per-entry links — not one link to the cache dir. + expect(fs.lstatSync(pluginSkills).isSymbolicLink()).toBe(false); + expect(fs.lstatSync(join(pluginSkills, "spec")).isSymbolicLink()).toBe(true); + expect(existsSync(join(pluginSkills, "evil"))).toBe(false); + // A stale link (skill removed upstream) is pruned on the next activation; + // a link pointing outside the cache is pruned too; a real dir is kept. + symlinkSync(join(cacheDir, "reg", "skills", "removed-skill"), join(pluginSkills, "removed-skill"), "dir"); + symlinkSync("/etc", join(pluginSkills, "outside"), "dir"); + mkdirSync(join(pluginSkills, "operator-owned"), { recursive: true }); + svc.resolveActivation("p"); + expect(fs.existsSync(join(pluginSkills, "removed-skill"))).toBe(false); + expect(() => fs.lstatSync(join(pluginSkills, "removed-skill"))).toThrow(); + expect(() => fs.lstatSync(join(pluginSkills, "outside"))).toThrow(); + expect(fs.lstatSync(join(pluginSkills, "operator-owned")).isDirectory()).toBe(true); + expect(existsSync(join(pluginSkills, "spec", "SKILL.md"))).toBe(true); + }); + test("skillScope: session — an UNTRUSTED pack gets no plugin (declaring is not executing)", async () => { const fixture = tmp(); writeRegistry(fixture, ["p"], ["spec"]); diff --git a/src/daemon/pipeline/pack-service.ts b/src/daemon/pipeline/pack-service.ts index 9601acc1..33631845 100644 --- a/src/daemon/pipeline/pack-service.ts +++ b/src/daemon/pipeline/pack-service.ts @@ -27,7 +27,7 @@ import { writeFileSync, } from "node:fs"; import { homedir } from "node:os"; -import { dirname, join, resolve } from "node:path"; +import { dirname, join, resolve, sep } from "node:path"; import type { AvailablePackWire, PackWire, RegistryWire } from "@highflame/codeoid-protocol"; import { type ModelBindingConfig, resolveBinding } from "./binding"; import type { Pack } from "./interface"; @@ -259,7 +259,8 @@ export class PackService { } } - /** The active skill scope (read by tests + `pack list` rendering). */ + /** The active skill scope — surfaced on the `pipeline.pack.list` snapshot so + * an operator can see which scope is live without opening config.json. */ get skillScope(): SkillScope { return this.#skillScope; } @@ -400,9 +401,21 @@ export class PackService { }); } - /** The combined pack state (the wire payload for pipeline.pack.list). */ - snapshot(): { installed: PackWire[]; available: AvailablePackWire[]; registries: RegistryWire[] } { - return { installed: this.installed(), available: this.available(), registries: this.listRegistries() }; + /** The combined pack state (the wire payload for pipeline.pack.list), plus + * the live skill scope so a client can tell how a trusted pack's skills + * reach sessions on this daemon (docs/pack-loading.md §3a). */ + snapshot(): { + installed: PackWire[]; + available: AvailablePackWire[]; + registries: RegistryWire[]; + skillScope: SkillScope; + } { + return { + installed: this.installed(), + available: this.available(), + registries: this.listRegistries(), + skillScope: this.#skillScope, + }; } // ── Mutations ─────────────────────────────────────────────────────────────── @@ -606,11 +619,25 @@ export class PackService { /** * Session-scoped skills (`skillScope: "session"`): materialize a Claude-Code- * plugin-shaped directory for a registry — `.claude-plugin/plugin.json` naming - * the registry, plus `skills` → `/skills` — and return it for the SDK - * `plugins` option. Idempotent and self-repairing (a stale `skills` link is - * re-pointed). Registries are data-only, so the manifest is synthesized here - * rather than required of them. Plugin skills resolve both bare (`/spec`) and - * namespaced (`/:spec`), so pack `command:` values need no rewrite. + * the registry, plus a real `skills/` dir holding ONE symlink per real skill + * directory in `/skills` — and return it for the SDK `plugins` option. + * + * Per-entry links, not one link to the whole cache dir, on purpose: the same + * lstat guard as #linkSkills. A registry could ship `skills/evil -> ~/.ssh`; + * a whole-dir link would expose that entry to discovery AND, because + * skillSandboxDirs widens the read sandbox to the real parent of every entry, + * grant the agent read access to `~` — strictly weaker than the global path, + * which never links a symlinked entry. Mirroring the guard keeps the two + * scopes equally strong. + * + * Idempotent and self-healing: links whose source vanished upstream (a skill + * removed on `registry refresh`) or that point outside the cache are pruned; + * a whole-dir `skills` symlink from a pre-release layout is replaced. A real + * skill directory an operator placed here is left alone. Registries are + * data-only, so the manifest is synthesized rather than required of them. + * Plugin skills resolve both bare (`/spec`) and namespaced + * (`/:spec`); a same-named skill in the user or project tier wins + * the bare form, exactly as it wins a global-scope link collision. * Returns undefined when the registry ships no `skills/` or the dir can't be * written (logged) — the activation then simply carries no plugin. */ @@ -618,6 +645,7 @@ export class PackService { const skillsSrc = join(this.#cachePath(registry), "skills"); if (!existsSync(skillsSrc)) return undefined; const pluginDir = join(this.#pluginsDir, registry); + const skillsDir = join(pluginDir, "skills"); try { mkdirSync(join(pluginDir, ".claude-plugin"), { recursive: true }); const manifestPath = join(pluginDir, ".claude-plugin", "plugin.json"); @@ -633,21 +661,42 @@ export class PackService { if (!existsSync(manifestPath) || readFileSync(manifestPath, "utf8") !== manifest) { writeFileSync(manifestPath, manifest); } - const link = join(pluginDir, "skills"); - let current: ReturnType | undefined; + // A whole-dir `skills` symlink (pre-release layout) is the exact shape the + // per-entry rule exists to prevent — replace it with a real directory. try { - current = lstatSync(link); + if (lstatSync(skillsDir).isSymbolicLink()) unlinkSync(skillsDir); } catch { - current = undefined; + /* absent — created below */ } - if (current?.isSymbolicLink()) { - if (readlinkSync(link) !== skillsSrc || !existsSync(link)) unlinkSync(link); - else return pluginDir; - } else if (current) { - // A real directory here is operator-managed — leave it, use it. - return pluginDir; + mkdirSync(skillsDir, { recursive: true }); + // Prune: a link we made whose source is gone, or that no longer points + // into this registry's cache. Real directories are operator-owned. + const srcPrefix = skillsSrc + sep; + for (const name of readdirSync(skillsDir)) { + const p = join(skillsDir, name); + try { + if (!lstatSync(p).isSymbolicLink()) continue; + if (!readlinkSync(p).startsWith(srcPrefix) || !existsSync(p)) unlinkSync(p); + } catch { + /* raced away — nothing to prune */ + } + } + // Link: one entry per REAL directory in the cache (lstat, not stat — a + // symlinked entry in the registry is never propagated). + for (const name of readdirSync(skillsSrc)) { + const from = join(skillsSrc, name); + const to = join(skillsDir, name); + try { + if (!lstatSync(from).isDirectory()) continue; + if (isDanglingSymlink(to)) unlinkSync(to); + else if (existsSync(to)) continue; + symlinkSync(from, to, "dir"); + } catch (e) { + console.warn( + `[packs] could not expose skill "${name}" from registry "${registry}": ${e instanceof Error ? e.message : String(e)}`, + ); + } } - symlinkSync(skillsSrc, link, "dir"); return pluginDir; } catch (e) { console.warn( diff --git a/src/daemon/pipeline/pack.test.ts b/src/daemon/pipeline/pack.test.ts index dce30c38..87d05072 100644 --- a/src/daemon/pipeline/pack.test.ts +++ b/src/daemon/pipeline/pack.test.ts @@ -161,6 +161,28 @@ phases: expect(run.phases[0]!.def).toMatchObject({ skill: "spec", gate: "tests_pass" }); }); + test("an explicit plan names a pack's skill/gate by its qualified id; a bare id is told what exists", () => { + const mgr = new PipelineManager(new PipelineStore(new Database(":memory:"))); + mgr.installPack(loadPack(fullPack())); + const base = { name: "x", accountId: "a", projectId: "p", createdBy: "u" }; + // Qualified ids resolve through the bare fallback (the id IS the registry key). + const ok = mgr.create({ + ...base, + phases: [{ id: "one", kind: "skill", skill: "aif-test/spec", gate: "aif-test/tests_pass" }], + }); + expect(ok.phases[0]!.def.skill).toBe("aif-test/spec"); + // A bare id no longer borrows an installed pack's entry (that borrowing was + // the cross-pack leakage); the error names the qualified ids instead. + expect(() => mgr.create({ ...base, phases: [{ id: "one", kind: "skill", skill: "spec" }] })).toThrow( + /unknown skill "spec" — installed packs declare it as "aif-test\/spec"/, + ); + expect(() => + mgr.create({ ...base, phases: [{ id: "one", kind: "noop", gate: "tests_pass" }] }), + ).toThrow(/unknown gate "tests_pass" — installed packs declare it as "aif-test\/tests_pass"/); + // Built-in gates still resolve bare for explicit plans. + expect(() => mgr.create({ ...base, phases: [{ id: "one", kind: "noop", gate: "always" }] })).not.toThrow(); + }); + test("kind defaults to 'skill' when a phase declares only a skill", () => { const dir = writePack(`schema: codeoid/pack@v1 id: p diff --git a/src/daemon/pipeline/scoped.ts b/src/daemon/pipeline/scoped.ts index 0378874a..f00111e2 100644 --- a/src/daemon/pipeline/scoped.ts +++ b/src/daemon/pipeline/scoped.ts @@ -35,3 +35,13 @@ export function resolveScoped( export const hasScoped = (reg: Registry, packId: string | undefined, id: string): boolean => resolveScoped(reg, packId, id) !== undefined; + +/** Every pack-scoped entry whose bare part is `id` — for the create-time error + * when an explicit plan names a pack's skill/gate by bare id: the message can + * say which qualified ids exist instead of a flat "unknown". */ +export const scopedMatches = (reg: Registry, id: string): string[] => + reg + .list() + .map((x) => x.id) + .filter((x) => x.endsWith(`/${id}`)) + .sort(); diff --git a/src/daemon/session-manager.ts b/src/daemon/session-manager.ts index 888ee60c..0e9141b7 100644 --- a/src/daemon/session-manager.ts +++ b/src/daemon/session-manager.ts @@ -2403,6 +2403,11 @@ mcpHub: this.#mcpHub, id: compiled.id, constitution: compiled.constitution, subagents: adoption ? adoption.subagents : compiled.subagents, + // ...and, under `skillScope: "session"`, the pack's skills too — a + // global-scope adoption sees them via ~/.claude/skills; dropping the + // plugin here would make the orchestrator the one session that can't + // run the methodology's slash skills. + ...(adoption?.skillsPluginDir ? { skillsPluginDir: adoption.skillsPluginDir } : {}), }; } @@ -2783,7 +2788,13 @@ mcpHub: this.#mcpHub, child, parent.id, childBrief(collaboration, child, adoption?.constitution), - adoptedRole ? { packId: adoption!.id, role: adoptedRole } : undefined, + adoptedRole + ? { + packId: adoption!.id, + role: adoptedRole, + ...(adoption!.skillsPluginDir ? { skillsPluginDir: adoption!.skillsPluginDir } : {}), + } + : undefined, ), // Autonomous with a bounded budget — the same posture dispatch gives // its workers, and for the same reason: NOBODY ATTACHES TO A CHILD. diff --git a/src/terminal/pack-format.ts b/src/terminal/pack-format.ts index a92e7391..46a8fdab 100644 --- a/src/terminal/pack-format.ts +++ b/src/terminal/pack-format.ts @@ -10,6 +10,15 @@ import type { AvailablePackWire, PackListResultMsg } from "../protocol/types.js" /** Render the installed / available / registries snapshot as console lines. */ export function formatPackList(res: PackListResultMsg): string[] { const out: string[] = []; + // Which way a trusted pack's skills reach sessions (docs/pack-loading.md + // §3a). Older daemons don't report it — print nothing rather than guess. + if (res.skillScope) { + const how = + res.skillScope === "session" + ? "per-session SDK plugin (nothing linked into ~/.claude/skills)" + : "symlinked into ~/.claude/skills (machine-wide)"; + out.push("", ` Skill scope: ${res.skillScope} — ${how}`); + } out.push("", " Registries:"); if (res.registries.length === 0) { out.push(" (none — add one: codeoid pack registry add )"); diff --git a/src/tests/collaboration.test.ts b/src/tests/collaboration.test.ts index 4f0e3a33..e155f44a 100644 --- a/src/tests/collaboration.test.ts +++ b/src/tests/collaboration.test.ts @@ -1000,6 +1000,20 @@ describe("pack-adopted posture + constitution (unit)", () => { expect(p.workerShape).toBe("scout"); }); + test("roleChildPosture carries the adopted pack's session-scoped skill plugin, and only then", () => { + const withPlugin = roleChildPosture(child, "parent", "brief", { + packId: "pk", + role: ADOPT_ROLES.adversary!, + skillsPluginDir: "/plugins/reg", + }); + expect(withPlugin.pack.skillsPluginDir).toBe("/plugins/reg"); + // Global scope (no plugin dir on the activation) and free-form both carry none. + const global = roleChildPosture(child, "parent", "brief", { packId: "pk", role: ADOPT_ROLES.adversary! }); + expect(global.pack.skillsPluginDir).toBeUndefined(); + expect("skillsPluginDir" in global.pack).toBe(false); + expect(roleChildPosture(child, "parent", "brief").pack.skillsPluginDir).toBeUndefined(); + }); + test("without adoption the synthesized free-form posture is unchanged", () => { const p = roleChildPosture(child, "parent", "brief"); expect(p.pack.id).toBe("collaboration"); diff --git a/src/tests/config.test.ts b/src/tests/config.test.ts index 73348289..88b78530 100644 --- a/src/tests/config.test.ts +++ b/src/tests/config.test.ts @@ -472,6 +472,27 @@ describe("loadConfig — hooks", () => { expect(off.pipeline?.enabled).toBe(false); }); + it("pipeline.skillScope defaults to global, accepts session, honors the env override, and rejects other values", () => { + writeConfig({}); + expect(loadConfig({ configPath, env: {} }).pipeline?.skillScope).toBe("global"); + + writeConfig({ pipeline: { skillScope: "session" } }); + expect(loadConfig({ configPath, env: {} }).pipeline?.skillScope).toBe("session"); + + // Per-invocation override wins over the file, in either direction. + expect( + loadConfig({ configPath, env: { CODEOID_PIPELINE_SKILL_SCOPE: "global" } }).pipeline?.skillScope, + ).toBe("global"); + writeConfig({}); + expect( + loadConfig({ configPath, env: { CODEOID_PIPELINE_SKILL_SCOPE: "session" } }).pipeline?.skillScope, + ).toBe("session"); + + // The enum is closed — a typo must fail loud, not silently fall back to global. + writeConfig({ pipeline: { skillScope: "sesion" } }); + expect(() => loadConfig({ configPath, env: {} })).toThrow(/skillScope|session|global/); + }); + it("pipeline.modelTiers / modelRoles parse, default to {}, and reject a missing provider", () => { writeConfig({}); const c = loadConfig({ configPath, env: {} });