From 8cf133557718238690b1fda9195a3e5b9a159b4d Mon Sep 17 00:00:00 2001 From: Ramnique Singh <30795890+ramnique@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:38:51 +0530 Subject: [PATCH] refactor(x): make capability activation a discriminated union; fence the loadSkill path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The design rule "app/always capabilities are never model-loadable" was held only by two arrays happening to be separate. Now it is structural: CapabilityDefinition is a union — ModelCapability (requires catalog title/summary and lazy content, cannot carry a fragment) vs EagerCapability (requires promptFragment, never enters the catalog) — and DiskSkill is typed as ModelCapability, making the disk trust boundary a compile-time fact instead of a comment. The kitchen is fenced along with the menu: catalog building and skill-tool attachment go through pure, fenced helpers (buildCatalogFromEntries / toolNamesFromEntries), and the boundary test now feeds a real eager capability through a mixed list and asserts both the catalog hides it and the tool path refuses its tools. Golden snapshots unchanged. Co-Authored-By: Claude Fable 5 --- .../core/src/agents/compose-instructions.ts | 2 +- .../assistant/capabilities/modes.ts | 4 +- .../assistant/capabilities/types.ts | 58 ++++++++++++------- .../assistant/capabilities/workspace.ts | 6 +- .../assistant/skills/disk-loader.ts | 22 ++++--- .../assistant/skills/index.test.ts | 54 +++++++++++++---- .../src/application/assistant/skills/index.ts | 56 +++++++++++++----- 7 files changed, 136 insertions(+), 66 deletions(-) diff --git a/apps/x/packages/core/src/agents/compose-instructions.ts b/apps/x/packages/core/src/agents/compose-instructions.ts index 2b30402e..a5d925c7 100644 --- a/apps/x/packages/core/src/agents/compose-instructions.ts +++ b/apps/x/packages/core/src/agents/compose-instructions.ts @@ -78,7 +78,7 @@ export function composeSystemInstructions({ coachMode: coachMode ?? false, }; for (const capability of PROMPT_CAPABILITIES) { - const fragment = capability.promptFragment?.(ctx); + const fragment = capability.promptFragment(ctx); if (fragment) { composed += `\n\n${fragment}`; } diff --git a/apps/x/packages/core/src/application/assistant/capabilities/modes.ts b/apps/x/packages/core/src/application/assistant/capabilities/modes.ts index e8641747..5d76c4c0 100644 --- a/apps/x/packages/core/src/application/assistant/capabilities/modes.ts +++ b/apps/x/packages/core/src/application/assistant/capabilities/modes.ts @@ -1,4 +1,4 @@ -import type { CapabilityContext, CapabilityDefinition } from "./types.js"; +import type { CapabilityContext, EagerCapability } from "./types.js"; // The app-activated capabilities: the modes the app (not the model) toggles — // facts about the world like "the camera is on" whose guidance must be in the @@ -11,7 +11,7 @@ import type { CapabilityContext, CapabilityDefinition } from "./types.js"; // code_agent_run/launch-code-task, but they stay in COPILOT_BASE_TOOLS until // the legacy runs engine (which cannot attach tools mid-run) is retired. -export const MODE_CAPABILITIES: readonly CapabilityDefinition[] = [ +export const MODE_CAPABILITIES: readonly EagerCapability[] = [ { id: "voice-input", activation: "app", diff --git a/apps/x/packages/core/src/application/assistant/capabilities/types.ts b/apps/x/packages/core/src/application/assistant/capabilities/types.ts index a3ebb7d5..033be196 100644 --- a/apps/x/packages/core/src/application/assistant/capabilities/types.ts +++ b/apps/x/packages/core/src/application/assistant/capabilities/types.ts @@ -11,11 +11,16 @@ // system prompt from token zero and tools attach at assembly. // - 'always' — unconditional contributions (workspace context traits). // +// The two shapes are a discriminated union so the design's rules are +// unrepresentable to break: a model capability MUST carry catalog metadata +// and lazy content and CANNOT carry an eager fragment; an app/always +// capability MUST carry a fragment and never appears in the catalog. +// // Trust boundary: disk-loaded skills (~/.rowboat/skills, ~/.agents/skills) -// are structurally limited to the 'model' subset — eager prompt fragments -// and app activation are bundled-only powers. A model-activated skill's -// prose lands in conversation because the model chose to read it; a disk -// file injecting into every turn's system prompt would be a standing +// are typed as ModelCapability, so eager prompt fragments and app activation +// are structurally bundled-only powers. A model-activated skill's prose +// lands in conversation because the model chose to read it; a disk file +// injecting into every turn's system prompt would be a standing // prompt-injection channel. export type CapabilityActivation = "model" | "app" | "always"; @@ -39,26 +44,37 @@ export interface CapabilityContext { coachMode: boolean; } -export interface CapabilityDefinition { +// Model-activated (a "skill"): advertised in the loadSkill catalog, loaded +// by the model's judgment, guidance delivered lazily as a tool result. +export interface ModelCapability { id: string; - // Catalog metadata: required for model-activated entries (rendered in - // the loadSkill catalog); app/always entries omit them — nothing - // consumes them there, and stale copies of fragment headings would - // only drift into misinformation. - title?: string; - summary?: string; - // Defaults to 'model' (the historical skill behavior). - activation?: CapabilityActivation; - // Lazy guidance returned by loadSkill (model activation). - content?: string; - // BuiltinTools keys this capability owns. + // The discriminant; omitted means 'model' (the historical default). + activation?: "model"; + // Catalog metadata — the loadSkill catalog renders both. + title: string; + summary: string; + // Lazy guidance returned by loadSkill. + content: string; + // BuiltinTools keys this capability owns; attached mid-turn on load. tools?: string[]; - // Eager system-prompt fragment (app/always activation). Returns null - // when the capability contributes nothing for this context. MUST be - // pure: same context, same bytes. - promptFragment?: (ctx: CapabilityContext) => string | null; } -export function isModelActivated(def: Pick): boolean { +// App/always-activated: an eager system-prompt contribution composed at +// assembly time. Bundled-only (see trust boundary above). +export interface EagerCapability { + id: string; + activation: "app" | "always"; + // Returns null when the capability contributes nothing for this + // context. MUST be pure: same context, same bytes. + promptFragment: (ctx: CapabilityContext) => string | null; + // BuiltinTools keys this capability owns; attached at assembly. + tools?: string[]; +} + +export type CapabilityDefinition = ModelCapability | EagerCapability; + +export function isModelActivated( + def: CapabilityDefinition, +): def is ModelCapability { return def.activation === undefined || def.activation === "model"; } diff --git a/apps/x/packages/core/src/application/assistant/capabilities/workspace.ts b/apps/x/packages/core/src/application/assistant/capabilities/workspace.ts index 1864ff5c..bd8db4fd 100644 --- a/apps/x/packages/core/src/application/assistant/capabilities/workspace.ts +++ b/apps/x/packages/core/src/application/assistant/capabilities/workspace.ts @@ -1,4 +1,4 @@ -import type { CapabilityContext, CapabilityDefinition } from "./types.js"; +import type { CapabilityContext, EagerCapability } from "./types.js"; // The always-activated workspace-context capabilities: agent notes and the // user work directory, composed for agents with the workspaceContext trait @@ -8,13 +8,13 @@ import type { CapabilityContext, CapabilityDefinition } from "./types.js"; // extracted verbatim from the historical composer; the golden snapshots in // agents/compose-instructions.test.ts pin the bytes. -export const AGENT_NOTES_CAPABILITY: CapabilityDefinition = { +export const AGENT_NOTES_CAPABILITY: EagerCapability = { id: "agent-notes", activation: "always", promptFragment: (ctx: CapabilityContext) => ctx.agentNotesContext, }; -export const WORK_DIRECTORY_CAPABILITY: CapabilityDefinition = { +export const WORK_DIRECTORY_CAPABILITY: EagerCapability = { id: "work-directory", activation: "always", promptFragment: (ctx: CapabilityContext) => diff --git a/apps/x/packages/core/src/application/assistant/skills/disk-loader.ts b/apps/x/packages/core/src/application/assistant/skills/disk-loader.ts index 8aec5d85..56b6670d 100644 --- a/apps/x/packages/core/src/application/assistant/skills/disk-loader.ts +++ b/apps/x/packages/core/src/application/assistant/skills/disk-loader.ts @@ -2,24 +2,22 @@ import fs from "node:fs"; import path from "node:path"; import { homedir } from "node:os"; import { WorkDir } from "../../../config/config.js"; +import type { ModelCapability } from "../capabilities/types.js"; import { splitFrontmatter } from "../../lib/parse-frontmatter.js"; /** - * A skill discovered on disk. Matches the bundled SkillDefinition shape - * (id/title/summary/content) plus provenance about where it came from. + * A skill discovered on disk: structurally the model-activated capability + * variant (id = slugified folder name, title = frontmatter `name`, summary = + * frontmatter `description`, content = full raw SKILL.md text, tools = + * frontmatter `tools:`/`allowed-tools:` BuiltinTools names — validated where + * descriptors are built; unknown names dropped there with a warning) plus + * provenance. Typed as ModelCapability on purpose: disk content can never + * carry app/always activation or an eager prompt fragment — that is the + * trust boundary (capabilities/types.ts), enforced by the type system. */ -export type DiskSkill = { - id: string; // slugified folder name - title: string; // frontmatter `name` - summary: string; // frontmatter `description` - content: string; // full raw SKILL.md text +export type DiskSkill = ModelCapability & { dir: string; // absolute path to the skill folder skillFile: string; // absolute path to the SKILL.md - // Optional frontmatter `tools:` (or the Agent Skills spec's - // `allowed-tools:`) — BuiltinTools names the skill attaches when loaded. - // Names are validated against the live catalog where descriptors are - // built; unknown names are dropped there with a warning. - tools?: string[]; }; // Locations scanned for /SKILL.md subfolders. The rowboat root is diff --git a/apps/x/packages/core/src/application/assistant/skills/index.test.ts b/apps/x/packages/core/src/application/assistant/skills/index.test.ts index 02f38e54..48784e27 100644 --- a/apps/x/packages/core/src/application/assistant/skills/index.test.ts +++ b/apps/x/packages/core/src/application/assistant/skills/index.test.ts @@ -147,22 +147,54 @@ describe("skills index (disk skill merge layer)", () => { }); }); -describe("capability record (types)", () => { - it("defaults to model activation — every bundled skill is model-activated", async () => { +describe("capability activation boundary", () => { + it("defaults to model activation; eager variants are excluded", async () => { const { isModelActivated } = await import("../capabilities/types.js"); - // Bundled definitions carry no activation field today; they are the - // model-activated subset by default and must stay in the catalog. - expect(isModelActivated({})).toBe(true); - expect(isModelActivated({ activation: "model" })).toBe(true); - expect(isModelActivated({ activation: "app" })).toBe(false); - expect(isModelActivated({ activation: "always" })).toBe(false); + const { MODE_CAPABILITIES } = await import("../capabilities/modes.js"); + const modelSkill = { + id: "m1", + title: "M1", + summary: "model skill", + content: "guidance", + }; + expect(isModelActivated(modelSkill)).toBe(true); + expect(isModelActivated({ ...modelSkill, activation: "model" as const })).toBe(true); + for (const eager of MODE_CAPABILITIES) { + expect(isModelActivated(eager)).toBe(false); + } }); - it("keeps app/always capabilities out of the loadSkill catalog", async () => { + it("fences the catalog and the tool lookup against eager capabilities", async () => { + // The real failure mode this pins: a future commit merges eager + // capabilities into the shared entry list — the catalog must hide them + // AND the loadSkill tool-attachment path must refuse them (hiding the + // menu entry is not enough; the kitchen must refuse to serve it). + const { buildCatalogFromEntries, toolNamesFromEntries } = await import("./index.js"); + const { MODE_CAPABILITIES } = await import("../capabilities/modes.js"); + const eager = { ...MODE_CAPABILITIES[0], tools: ["file-writeText"] }; + const model = { + id: "real-skill", + title: "Real Skill", + summary: "a genuine model-activated skill", + content: "guidance", + tools: ["file-readText"], + catalogPath: "src/application/assistant/skills/real-skill/skill.ts", + }; + const mixed = [model, eager]; + + const catalog = buildCatalogFromEntries(mixed); + expect(catalog).toContain("real-skill"); + expect(catalog).not.toContain(eager.id); + + expect(toolNamesFromEntries(mixed, "real-skill")).toEqual(["file-readText"]); + // The eager capability's tools must NOT attach via the loadSkill path, + // even though the entry is present in the list and declares tools. + expect(toolNamesFromEntries(mixed, eager.id)).toEqual([]); + }); + + it("every advertised catalog id resolves via loadSkill", async () => { const { buildSkillCatalog, availableSkills } = await import("./index.js"); const catalog = buildSkillCatalog(); - // Every advertised id resolves via loadSkill (the catalog and the - // resolver operate on the same model-activated subset). for (const id of availableSkills) { expect(catalog).toContain(id); } diff --git a/apps/x/packages/core/src/application/assistant/skills/index.ts b/apps/x/packages/core/src/application/assistant/skills/index.ts index 9848c53b..bcbfc5e2 100644 --- a/apps/x/packages/core/src/application/assistant/skills/index.ts +++ b/apps/x/packages/core/src/application/assistant/skills/index.ts @@ -1,6 +1,10 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; -import { isModelActivated, type CapabilityDefinition } from "../capabilities/types.js"; +import { + isModelActivated, + type CapabilityDefinition, + type ModelCapability, +} from "../capabilities/types.js"; import { loadDiskSkills } from "./disk-loader.js"; import builtinToolsSkill from "./builtin-tools/skill.js"; import deletionGuardrailsSkill from "./deletion-guardrails/skill.js"; @@ -26,17 +30,12 @@ const CATALOG_PREFIX = "src/application/assistant/skills"; // console.log(liveNoteSkill); -// A skill is the model-activated subset of the capability record +// A skill IS the model-activated variant of the capability record // (capabilities/types.ts): lazy guidance the model pulls in via loadSkill, -// plus the BuiltinTools it owns. `id` doubles as the folder name; title and -// summary are required here because the catalog renders them. Tool names +// plus the BuiltinTools it owns. `id` doubles as the folder name. Tool names // are validated against the live catalog where descriptors are built // (loadSkill / the agent resolver); unknown names are dropped there. -type SkillDefinition = CapabilityDefinition & { - title: string; - summary: string; - content: string; -}; +type SkillDefinition = ModelCapability; type ResolvedSkill = { id: string; @@ -243,16 +242,27 @@ function getSkillEntries(): SkillEntry[] { * Reads the live skill set, so it reflects disk skills added/removed at runtime. */ export function buildSkillCatalog(options?: { excludeIds?: string[] }): string { - // The catalog advertises the model-activated subset only: app/always - // capabilities are composed into the system prompt by the assembly layer, - // not offered for loadSkill. - const modelEntries = getSkillEntries().filter(isModelActivated); + return buildCatalogFromEntries(getSkillEntries(), options); +} + +// Pure catalog builder over an explicit entry list (exported for the +// activation-boundary tests). The catalog advertises the model-activated +// subset only: app/always capabilities are composed into the system prompt +// by the assembly layer, never offered for loadSkill. +export function buildCatalogFromEntries( + all: ReadonlyArray, + options?: { excludeIds?: string[] }, +): string { + // The type-guard filter must keep the catalogPath intersection alive. + const modelEntries = all.filter( + (e): e is ModelCapability & { catalogPath?: string } => isModelActivated(e), + ); const entries = options?.excludeIds ? modelEntries.filter(e => !options.excludeIds!.includes(e.id)) : modelEntries; const sections = entries.map((entry) => [ `## ${entry.title}`, - `- **Skill file:** \`${entry.catalogPath}\``, + `- **Skill file:** \`${entry.catalogPath ?? `${CATALOG_PREFIX}/${entry.id}/skill.ts`}\``, `- **Use it for:** ${entry.summary}`, ...(entry.tools && entry.tools.length > 0 ? [`- **Tools it attaches:** ${entry.tools.join(", ")}`] @@ -384,8 +394,22 @@ export function resolveSkill(identifier: string): ResolvedSkill | null { * builtin-tools escape-hatch list. */ export function skillToolNames(skillId: string): string[] { - const entry = getSkillEntries().find((e) => e.id === skillId); - return entry?.tools ? [...entry.tools] : []; + return toolNamesFromEntries(getSkillEntries(), skillId); +} + +// Pure variant over an explicit entry list (exported for the +// activation-boundary tests). Only model-activated entries may attach tools +// through the loadSkill path — an eager capability's tools attach at +// assembly, never mid-turn on the model's request. +export function toolNamesFromEntries( + entries: ReadonlyArray, + skillId: string, +): string[] { + const entry = entries.find((e) => e.id === skillId); + if (!entry || !isModelActivated(entry)) { + return []; + } + return entry.tools ? [...entry.tools] : []; } /**