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 1/5] 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] : []; } /** From d0018daad33e409720a5be18b9ddb4066565b972 Mon Sep 17 00:00:00 2001 From: Ramnique Singh <30795890+ramnique@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:43:24 +0530 Subject: [PATCH 2/5] refactor(x): one ModeFlags schema behind all three composition shapes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The app-toggled mode flags were hand-mirrored in three places (the resolver's CompositionOverrides zod schema, CapabilityContext, and ComposeSystemInstructionsInput) and copied field-by-field with '?? false' defaults — so a mode missed at one site compiled clean and silently never composed. Now capabilities/types.ts declares one zod ModeFlags schema with defaults: the wire shape (all-optional, what turn files carry), the resolver's concrete parse output, and the composition context all derive from it; CompositionOverrides just extends it with the resolver-only keys, the composer takes {instructions, notes, workDir} & ModeFlags with a rest-spread (no copy list), and the legacy call site passes explicit false for the modes it never composes. Adding a mode = one schema key + one record; anything less is a compile/parse error. New resolver test pins historical-composition compatibility (sparse, null-heavy, unknown-key, and garbage compositions all compose exactly as before). Golden snapshots unchanged. Co-Authored-By: Claude Fable 5 --- .../core/src/agents/compose-instructions.ts | 50 +++++++------------ apps/x/packages/core/src/agents/runtime.ts | 3 ++ .../assistant/capabilities/types.ts | 39 ++++++++++----- .../turns/bridges/real-agent-resolver.test.ts | 40 +++++++++++++++ .../src/turns/bridges/real-agent-resolver.ts | 38 ++++++-------- 5 files changed, 102 insertions(+), 68 deletions(-) diff --git a/apps/x/packages/core/src/agents/compose-instructions.ts b/apps/x/packages/core/src/agents/compose-instructions.ts index a5d925c7..3a43c964 100644 --- a/apps/x/packages/core/src/agents/compose-instructions.ts +++ b/apps/x/packages/core/src/agents/compose-instructions.ts @@ -3,7 +3,10 @@ import { AGENT_NOTES_CAPABILITY, WORK_DIRECTORY_CAPABILITY, } from "../application/assistant/capabilities/workspace.js"; -import type { CapabilityContext } from "../application/assistant/capabilities/types.js"; +import type { + CapabilityContext, + ModeFlags, +} from "../application/assistant/capabilities/types.js"; // Everything that composes into the system prompt, in composition order. const PROMPT_CAPABILITIES = [ @@ -33,50 +36,31 @@ If Middle pane state is note, the supplied path and content are available so you If Middle pane state is browser, only the URL and page title are supplied; the page content itself is NOT included. If you need the page content to answer, use the browser tools available to you to read the page. The user may or may not be talking about this page. Only reference or act on this page when the user's message clearly relates to it, such as "this page", "this article", "what I'm looking at", "this site", or "summarize this". For unrelated questions, ignore this page entirely and answer normally. Do not mention that you can see the browser unless it is relevant to the answer.`; -export interface ComposeSystemInstructionsInput { +// The mode flags come straight from the shared ModeFlags shape (all +// required — callers normalize via ModeFlags.parse or pass explicit +// values), so a mode added to the schema is a compile error at every call +// site until it is threaded through, never a silently-absent prompt block. +export type ComposeSystemInstructionsInput = { instructions: string; agentNotesContext: string | null; userWorkDir: string | null; - voiceInput: boolean; - voiceOutput: 'summary' | 'full' | null; - searchEnabled: boolean; - codeMode: 'claude' | 'codex' | null; - codeCwd: string | null; - // Optional so legacy callers (old streamAgent path) are unaffected. - videoMode?: boolean; - coachMode?: boolean; -} +} & ModeFlags; -// System-prompt assembly, extracted verbatim from streamAgent so the new turn -// runtime's agent resolver composes byte-identical prompts. Pure: callers -// load agent notes / work dir themselves. +// System-prompt assembly: base instructions + hidden-user-context + the +// capability fragments. Pure: callers load agent notes / work dir +// themselves. export function composeSystemInstructions({ instructions, agentNotesContext, userWorkDir, - voiceInput, - voiceOutput, - searchEnabled, - codeMode, - codeCwd, - videoMode, - coachMode, + ...modeFlags }: ComposeSystemInstructionsInput): string { let composed = `${instructions}\n\n${USER_CONTEXT_SYSTEM_INSTRUCTIONS}`; // Capabilities compose in PROMPT_CAPABILITIES order — a fixed total // order, so identical inputs yield identical bytes. The fragment text - // lives with the capability records. - const ctx: CapabilityContext = { - agentNotesContext, - userWorkDir, - voiceInput, - voiceOutput, - searchEnabled, - codeMode, - codeCwd, - videoMode: videoMode ?? false, - coachMode: coachMode ?? false, - }; + // lives with the capability records; the rest-spread means new schema + // keys flow through without a hand-maintained copy list. + const ctx: CapabilityContext = { agentNotesContext, userWorkDir, ...modeFlags }; for (const capability of PROMPT_CAPABILITIES) { const fragment = capability.promptFragment(ctx); if (fragment) { diff --git a/apps/x/packages/core/src/agents/runtime.ts b/apps/x/packages/core/src/agents/runtime.ts index ac68c3ee..53534fd0 100644 --- a/apps/x/packages/core/src/agents/runtime.ts +++ b/apps/x/packages/core/src/agents/runtime.ts @@ -1322,6 +1322,9 @@ export async function* streamAgent({ searchEnabled, codeMode, codeCwd, + // The legacy runs engine never composes video/coach modes. + videoMode: false, + coachMode: false, }); let streamError: string | null = null; for await (const event of streamLlm( 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 033be196..1472a428 100644 --- a/apps/x/packages/core/src/application/assistant/capabilities/types.ts +++ b/apps/x/packages/core/src/application/assistant/capabilities/types.ts @@ -23,25 +23,38 @@ // injecting into every turn's system prompt would be a standing // prompt-injection channel. +import { z } from "zod"; + export type CapabilityActivation = "model" | "app" | "always"; -// Inputs an eager fragment may read. All fields mirror persisted -// RequestedAgent.overrides.composition values, resolved before composition — -// a fragment must be a pure function of this context so composed prompts -// stay byte-identical for identical inputs (provider prefix caching and -// agent-snapshot inheritance both depend on it). -export interface CapabilityContext { +// THE single source of truth for the app-toggled mode flags. Every key is a +// persisted RequestedAgent.overrides.composition value; defaults make the +// parsed output fully concrete, so the wire shape (all-optional, what turn +// files carry), the resolver's parse result, and the composition context are +// one declaration. Adding a mode = one key here + one record in modes.ts; +// forgetting anything else is a compile error, not a silently-absent prompt +// block. Every key alters system-prompt bytes, so prompt-affecting inputs +// must stay session-sticky (prefix caching). +export const ModeFlags = z.object({ + voiceInput: z.boolean().default(false), + voiceOutput: z.enum(["summary", "full"]).nullable().default(null), + searchEnabled: z.boolean().default(false), + codeMode: z.enum(["claude", "codex"]).nullable().default(null), + codeCwd: z.string().nullable().default(null), + videoMode: z.boolean().default(false), + coachMode: z.boolean().default(false), +}); +export type ModeFlags = z.infer; + +// Inputs an eager fragment may read: the mode flags plus resolved workspace +// context. A fragment must be a pure function of this context so composed +// prompts stay byte-identical for identical inputs (provider prefix caching +// and agent-snapshot inheritance both depend on it). +export interface CapabilityContext extends ModeFlags { // Workspace context (workspaceContext trait agents only; the resolver // loads these and leaves them null for everyone else). agentNotesContext: string | null; userWorkDir: string | null; - voiceInput: boolean; - voiceOutput: "summary" | "full" | null; - searchEnabled: boolean; - codeMode: "claude" | "codex" | null; - codeCwd: string | null; - videoMode: boolean; - coachMode: boolean; } // Model-activated (a "skill"): advertised in the loadSkill catalog, loaded diff --git a/apps/x/packages/core/src/turns/bridges/real-agent-resolver.test.ts b/apps/x/packages/core/src/turns/bridges/real-agent-resolver.test.ts index 1f7f46ec..0f0e090f 100644 --- a/apps/x/packages/core/src/turns/bridges/real-agent-resolver.test.ts +++ b/apps/x/packages/core/src/turns/bridges/real-agent-resolver.test.ts @@ -289,3 +289,43 @@ describe("active skills (skill-scoped tools)", () => { expect(resolved.tools).toEqual([]); }); }); + +describe("historical composition compatibility", () => { + // Persisted RequestedAgent.overrides.composition values are replayed + // from turn files; the ModeFlags-derived schema must keep accepting + // every historical shape byte-for-byte. + it("composes identically for sparse, null-heavy, and unknown-key compositions", async () => { + const resolver = makeResolver(makeAgent()); + const sparse = await resolver.resolve({ + agentId: "copilot", + overrides: { composition: { voiceInput: true } }, + }); + const explicit = await resolver.resolve({ + agentId: "copilot", + overrides: { + composition: { + voiceInput: true, + voiceOutput: null, + searchEnabled: false, + codeMode: null, + codeCwd: null, + videoMode: false, + coachMode: false, + workDirId: null, + // Unknown keys have always been ignored. + futureUnknownKey: "ignored", + }, + }, + }); + expect(sparse.systemPrompt).toBe(explicit.systemPrompt); + expect(sparse.systemPrompt).toContain("# Voice Input"); + + // Garbage compositions fall back to all-defaults, not a throw. + const garbage = await resolver.resolve({ + agentId: "copilot", + overrides: { composition: { voiceInput: "yes" } }, + }); + const none = await resolver.resolve({ agentId: "copilot" }); + expect(garbage.systemPrompt).toBe(none.systemPrompt); + }); +}); diff --git a/apps/x/packages/core/src/turns/bridges/real-agent-resolver.ts b/apps/x/packages/core/src/turns/bridges/real-agent-resolver.ts index 24dd787e..6992e958 100644 --- a/apps/x/packages/core/src/turns/bridges/real-agent-resolver.ts +++ b/apps/x/packages/core/src/turns/bridges/real-agent-resolver.ts @@ -14,6 +14,7 @@ import { import { hasWorkspaceContext, loadAgent } from "../../agents/registry.js"; import { BuiltinTools } from "../../application/lib/builtin-tools.js"; import { skillToolNames } from "../../application/assistant/skills/index.js"; +import { ModeFlags } from "../../application/assistant/capabilities/types.js"; import { getDefaultModelAndProvider } from "../../models/defaults.js"; import { builtinToolDescriptor, @@ -51,15 +52,11 @@ const ASK_HUMAN_DESCRIPTOR: z.infer = { // Unknown keys are ignored. Prompt-affecting inputs should be session-sticky: // every key here alters system-prompt bytes and therefore busts provider // prefix caching when it changes between turns. -const CompositionOverrides = z.object({ +// The mode-flag keys come from the shared ModeFlags schema (the single +// source of truth in capabilities/types.ts — defaults make the parse output +// fully concrete); only the resolver-specific keys are declared here. +const CompositionOverrides = ModeFlags.extend({ workDirId: z.string().nullable().optional(), - voiceInput: z.boolean().optional(), - voiceOutput: z.enum(["summary", "full"]).nullable().optional(), - searchEnabled: z.boolean().optional(), - codeMode: z.enum(["claude", "codex"]).nullable().optional(), - codeCwd: z.string().nullable().optional(), - videoMode: z.boolean().optional(), - coachMode: z.boolean().optional(), // Set by spawn-agent for by-id children: strips the spawn tool so depth // is capped at 1 regardless of which stored agent is spawned. subagent: z.boolean().optional(), @@ -123,7 +120,12 @@ export class RealAgentResolver { const parsed = CompositionOverrides.safeParse( requested.overrides?.composition ?? {}, ); - const composition = parsed.success ? parsed.data : {}; + // An unparseable composition falls back to all defaults (same shape + // as parsing {}), preserving the historical "ignore garbage" rule. + const composition = parsed.success + ? parsed.data + : CompositionOverrides.parse({}); + const { workDirId, subagent, activeSkills, ...modeFlags } = composition; // Agent notes and work-dir context are scoped to agents with the // workspaceContext trait (the copilot), per the agent registry. const copilotContext = hasWorkspaceContext(requested.agentId); @@ -131,23 +133,15 @@ export class RealAgentResolver { instructions: agent.instructions, agentNotesContext: copilotContext ? this.loadNotes() : null, userWorkDir: - copilotContext && composition.workDirId - ? this.loadWorkDir(composition.workDirId) - : null, - voiceInput: composition.voiceInput ?? false, - voiceOutput: composition.voiceOutput ?? null, - searchEnabled: composition.searchEnabled ?? false, - codeMode: composition.codeMode ?? null, - codeCwd: composition.codeCwd ?? null, - videoMode: composition.videoMode ?? false, - coachMode: composition.coachMode ?? false, + copilotContext && workDirId ? this.loadWorkDir(workDirId) : null, + ...modeFlags, }); const tools = await this.resolveTools(agent, { - subagent: composition.subagent ?? false, + subagent: subagent ?? false, }); - if (copilotContext && composition.activeSkills?.length) { - await this.appendSkillTools(tools, composition.activeSkills); + if (copilotContext && activeSkills?.length) { + await this.appendSkillTools(tools, activeSkills); } return ResolvedAgent.parse({ agentId: requested.agentId, From efeaae1e19c6bc00033f346e2ad6f9d205124821 Mon Sep 17 00:00:00 2001 From: Ramnique Singh <30795890+ramnique@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:45:58 +0530 Subject: [PATCH 3/5] refactor(x): workspace-context loading gets one trait-gated chokepoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workspaceContext trait was consulted separately at every assembly site before loading agent notes / work dir — a convention each caller had to re-remember, where a forgotten check either leaks the user's agent-memory into non-copilot prompts or silently omits it for the copilot, and neither fails loudly. The loaders move out of the legacy runtime file into agents/workspace-context.ts, and loadWorkspaceContext() applies the trait gate INSIDE: both engines now call it (the resolver keeps its loader test-seams by injecting them through), so a future assembly site structurally cannot skip the gate. Tests pin the gate: non-workspace agents get nulls even when the loaders would yield values. Co-Authored-By: Claude Fable 5 --- apps/x/packages/core/src/agents/runtime.ts | 80 +----------- .../core/src/agents/workspace-context.test.ts | 41 +++++++ .../core/src/agents/workspace-context.ts | 115 ++++++++++++++++++ .../src/turns/bridges/real-agent-resolver.ts | 15 ++- 4 files changed, 169 insertions(+), 82 deletions(-) create mode 100644 apps/x/packages/core/src/agents/workspace-context.test.ts create mode 100644 apps/x/packages/core/src/agents/workspace-context.ts diff --git a/apps/x/packages/core/src/agents/runtime.ts b/apps/x/packages/core/src/agents/runtime.ts index 53534fd0..d4d9b9c3 100644 --- a/apps/x/packages/core/src/agents/runtime.ts +++ b/apps/x/packages/core/src/agents/runtime.ts @@ -12,6 +12,7 @@ import { TOOL_ADDITIONS_KEY } from "../application/lib/tool-additions.js"; import { AskHumanRequestEvent, RunEvent, ToolPermissionMetadata, ToolPermissionRequestEvent } from "@x/shared/dist/runs.js"; import { BuiltinTools } from "../application/lib/builtin-tools.js"; import { hasWorkspaceContext, loadAgent } from "./registry.js"; +import { loadWorkspaceContext } from "./workspace-context.js"; import { isBlocked, extractCommandNames } from "../application/lib/command-executor.js"; import { getFileAccessAllowList, type FileAccessGrant, type FileAccessOperation } from "../config/security.js"; import { resolveFilePathForPermission } from "../filesystem/files.js"; @@ -31,14 +32,6 @@ import { captureLlmUsage } from "../analytics/usage.js"; import { enterUseCase, withUseCase, type UseCase } from "../analytics/use_case.js"; import { classifyToolPermissions, type AutoPermissionCandidate } from "../security/auto-permission-classifier.js"; -const AGENT_NOTES_DIR = path.join(WorkDir, 'knowledge', 'Agent Notes'); - -// Work directory is scoped per run (per chat). Each run gets its own sidecar -// config file so setting it in one chat does not leak into others. -function workDirConfigFile(runId: string): string { - return path.join(WorkDir, 'config', `workdir-${runId}.json`); -} - type ToolPermissionMetadataValue = z.infer; function isPathInside(parent: string, child: string): boolean { @@ -164,71 +157,6 @@ export async function getToolPermissionMetadata( }; } -export function loadUserWorkDir(runId: string): string | null { - try { - const file = workDirConfigFile(runId); - if (!fs.existsSync(file)) return null; - const raw = fs.readFileSync(file, 'utf-8'); - const parsed = JSON.parse(raw) as { path?: unknown }; - const value = typeof parsed.path === 'string' ? parsed.path.trim() : ''; - return value || null; - } catch { - return null; - } -} - -export function loadAgentNotesContext(): string | null { - const sections: string[] = []; - - const userFile = path.join(AGENT_NOTES_DIR, 'user.md'); - const prefsFile = path.join(AGENT_NOTES_DIR, 'preferences.md'); - - try { - if (fs.existsSync(userFile)) { - const content = fs.readFileSync(userFile, 'utf-8').trim(); - if (content) { - sections.push(`## About the User\nThese are notes you took about the user in previous chats.\n\n${content}`); - } - } - } catch { /* ignore */ } - - try { - if (fs.existsSync(prefsFile)) { - const content = fs.readFileSync(prefsFile, 'utf-8').trim(); - if (content) { - sections.push(`## User Preferences\nThese are notes you took on their general preferences.\n\n${content}`); - } - } - } catch { /* ignore */ } - - // List other Agent Notes files for on-demand access - const otherFiles: string[] = []; - const skipFiles = new Set(['user.md', 'preferences.md', 'inbox.md']); - try { - if (fs.existsSync(AGENT_NOTES_DIR)) { - function listMdFiles(dir: string, prefix: string) { - for (const entry of fs.readdirSync(dir)) { - const fullPath = path.join(dir, entry); - const stat = fs.statSync(fullPath); - if (stat.isDirectory()) { - listMdFiles(fullPath, `${prefix}${entry}/`); - } else if (entry.endsWith('.md') && !skipFiles.has(`${prefix}${entry}`)) { - otherFiles.push(`${prefix}${entry}`); - } - } - } - listMdFiles(AGENT_NOTES_DIR, ''); - } - } catch { /* ignore */ } - - if (otherFiles.length > 0) { - sections.push(`## More Specific Preferences\nFor more specific preferences, you can read these files using file-readText. Only read them when relevant to the current task.\n\n${otherFiles.map(f => `- knowledge/Agent Notes/${f}`).join('\n')}`); - } - - if (sections.length === 0) return null; - return `# Agent Memory\n\n${sections.join('\n\n')}`; -} - function formatCurrentDateTime(now: Date): string { return now.toLocaleString('en-US', { weekday: 'long', @@ -1312,11 +1240,11 @@ export async function* streamAgent({ loopLogger.log('running llm turn'); // stream agent response and build message const messageBuilder = new StreamStepMessageBuilder(); - const composeCopilotContext = hasWorkspaceContext(state.agentName); + const workspace = loadWorkspaceContext(state.agentName, runId); const instructionsWithDateTime = composeSystemInstructions({ instructions: agent.instructions, - agentNotesContext: composeCopilotContext ? loadAgentNotesContext() : null, - userWorkDir: composeCopilotContext ? loadUserWorkDir(runId) : null, + agentNotesContext: workspace.agentNotesContext, + userWorkDir: workspace.userWorkDir, voiceInput, voiceOutput, searchEnabled, diff --git a/apps/x/packages/core/src/agents/workspace-context.test.ts b/apps/x/packages/core/src/agents/workspace-context.test.ts new file mode 100644 index 00000000..a185b6db --- /dev/null +++ b/apps/x/packages/core/src/agents/workspace-context.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, it } from "vitest"; +import { loadWorkspaceContext } from "./workspace-context.js"; + +const loaders = { + loadNotes: () => "# Agent Memory\n\nnotes", + loadWorkDir: (key: string) => (key === "chat-1" ? "/Users/me/work" : null), +}; + +describe("loadWorkspaceContext", () => { + it("loads notes and work dir for workspaceContext-trait agents", () => { + expect(loadWorkspaceContext("copilot", "chat-1", loaders)).toEqual({ + agentNotesContext: "# Agent Memory\n\nnotes", + userWorkDir: "/Users/me/work", + }); + // rowboatx is the copilot alias. + expect(loadWorkspaceContext("rowboatx", "chat-1", loaders).userWorkDir).toBe( + "/Users/me/work", + ); + }); + + it("returns nulls for non-workspace agents even when the loaders would yield values", () => { + // THE gate this module exists for: a caller cannot leak the user's + // agent-memory into a background/knowledge agent's prompt by + // forgetting a trait check — the check lives here. + for (const agentId of ["background-task-agent", "note_creation", "some-user-agent"]) { + expect(loadWorkspaceContext(agentId, "chat-1", loaders)).toEqual({ + agentNotesContext: null, + userWorkDir: null, + }); + } + expect(loadWorkspaceContext(null, "chat-1", loaders).agentNotesContext).toBeNull(); + expect(loadWorkspaceContext(undefined, "chat-1", loaders).agentNotesContext).toBeNull(); + }); + + it("skips the work-dir lookup without a key", () => { + expect(loadWorkspaceContext("copilot", null, loaders)).toEqual({ + agentNotesContext: "# Agent Memory\n\nnotes", + userWorkDir: null, + }); + }); +}); diff --git a/apps/x/packages/core/src/agents/workspace-context.ts b/apps/x/packages/core/src/agents/workspace-context.ts new file mode 100644 index 00000000..ea0039e1 --- /dev/null +++ b/apps/x/packages/core/src/agents/workspace-context.ts @@ -0,0 +1,115 @@ +import fs from "fs"; +import path from "path"; +import { WorkDir } from "../config/config.js"; +import { hasWorkspaceContext } from "./registry.js"; + +// Workspace context for agent assembly: agent notes (global) and the user's +// per-chat work directory. loadWorkspaceContext is THE chokepoint — the +// workspaceContext trait gate lives inside it, so no assembly site can +// forget the gate and leak the user's agent-memory into a non-workspace +// agent's prompt (or silently omit it for the copilot). Loaders extracted +// verbatim from the legacy runtime file. + +const AGENT_NOTES_DIR = path.join(WorkDir, 'knowledge', 'Agent Notes'); + +// Work directory is scoped per run (per chat). Each run gets its own sidecar +// config file so setting it in one chat does not leak into others. +function workDirConfigFile(runId: string): string { + return path.join(WorkDir, 'config', `workdir-${runId}.json`); +} + +export function loadUserWorkDir(runId: string): string | null { + try { + const file = workDirConfigFile(runId); + if (!fs.existsSync(file)) return null; + const raw = fs.readFileSync(file, 'utf-8'); + const parsed = JSON.parse(raw) as { path?: unknown }; + const value = typeof parsed.path === 'string' ? parsed.path.trim() : ''; + return value || null; + } catch { + return null; + } +} + +export function loadAgentNotesContext(): string | null { + const sections: string[] = []; + + const userFile = path.join(AGENT_NOTES_DIR, 'user.md'); + const prefsFile = path.join(AGENT_NOTES_DIR, 'preferences.md'); + + try { + if (fs.existsSync(userFile)) { + const content = fs.readFileSync(userFile, 'utf-8').trim(); + if (content) { + sections.push(`## About the User\nThese are notes you took about the user in previous chats.\n\n${content}`); + } + } + } catch { /* ignore */ } + + try { + if (fs.existsSync(prefsFile)) { + const content = fs.readFileSync(prefsFile, 'utf-8').trim(); + if (content) { + sections.push(`## User Preferences\nThese are notes you took on their general preferences.\n\n${content}`); + } + } + } catch { /* ignore */ } + + // List other Agent Notes files for on-demand access + const otherFiles: string[] = []; + const skipFiles = new Set(['user.md', 'preferences.md', 'inbox.md']); + try { + if (fs.existsSync(AGENT_NOTES_DIR)) { + function listMdFiles(dir: string, prefix: string) { + for (const entry of fs.readdirSync(dir)) { + const fullPath = path.join(dir, entry); + const stat = fs.statSync(fullPath); + if (stat.isDirectory()) { + listMdFiles(fullPath, `${prefix}${entry}/`); + } else if (entry.endsWith('.md') && !skipFiles.has(`${prefix}${entry}`)) { + otherFiles.push(`${prefix}${entry}`); + } + } + } + listMdFiles(AGENT_NOTES_DIR, ''); + } + } catch { /* ignore */ } + + if (otherFiles.length > 0) { + sections.push(`## More Specific Preferences\nFor more specific preferences, you can read these files using file-readText. Only read them when relevant to the current task.\n\n${otherFiles.map(f => `- knowledge/Agent Notes/${f}`).join('\n')}`); + } + + if (sections.length === 0) return null; + return `# Agent Memory\n\n${sections.join('\n\n')}`; +} + +export interface WorkspaceContext { + agentNotesContext: string | null; + userWorkDir: string | null; +} + +const NO_WORKSPACE: WorkspaceContext = { + agentNotesContext: null, + userWorkDir: null, +}; + +// workDirKey is the per-chat work-directory sidecar key: the composition's +// workDirId on the turn runtime, the runId on the legacy engine. +export function loadWorkspaceContext( + agentId: string | null | undefined, + workDirKey: string | null | undefined, + loaders: { + loadNotes?: typeof loadAgentNotesContext; + loadWorkDir?: typeof loadUserWorkDir; + } = {}, +): WorkspaceContext { + if (!hasWorkspaceContext(agentId)) { + return NO_WORKSPACE; + } + const loadNotes = loaders.loadNotes ?? loadAgentNotesContext; + const loadWorkDir = loaders.loadWorkDir ?? loadUserWorkDir; + return { + agentNotesContext: loadNotes(), + userWorkDir: workDirKey ? loadWorkDir(workDirKey) : null, + }; +} diff --git a/apps/x/packages/core/src/turns/bridges/real-agent-resolver.ts b/apps/x/packages/core/src/turns/bridges/real-agent-resolver.ts index 6992e958..635906b9 100644 --- a/apps/x/packages/core/src/turns/bridges/real-agent-resolver.ts +++ b/apps/x/packages/core/src/turns/bridges/real-agent-resolver.ts @@ -10,7 +10,8 @@ import { composeSystemInstructions } from "../../agents/compose-instructions.js" import { loadAgentNotesContext, loadUserWorkDir, -} from "../../agents/runtime.js"; + loadWorkspaceContext, +} from "../../agents/workspace-context.js"; import { hasWorkspaceContext, loadAgent } from "../../agents/registry.js"; import { BuiltinTools } from "../../application/lib/builtin-tools.js"; import { skillToolNames } from "../../application/assistant/skills/index.js"; @@ -126,14 +127,16 @@ export class RealAgentResolver { ? parsed.data : CompositionOverrides.parse({}); const { workDirId, subagent, activeSkills, ...modeFlags } = composition; - // Agent notes and work-dir context are scoped to agents with the - // workspaceContext trait (the copilot), per the agent registry. + // The workspaceContext trait gate lives INSIDE loadWorkspaceContext + // (agents/workspace-context.ts) — no assembly site can forget it. + const workspace = loadWorkspaceContext(requested.agentId, workDirId, { + loadNotes: this.loadNotes, + loadWorkDir: this.loadWorkDir, + }); const copilotContext = hasWorkspaceContext(requested.agentId); const systemPrompt = composeSystemInstructions({ instructions: agent.instructions, - agentNotesContext: copilotContext ? this.loadNotes() : null, - userWorkDir: - copilotContext && workDirId ? this.loadWorkDir(workDirId) : null, + ...workspace, ...modeFlags, }); From 8dd5a2b49fda8e67a3f9173fd70752011af5578f Mon Sep 17 00:00:00 2001 From: Ramnique Singh <30795890+ramnique@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:48:24 +0530 Subject: [PATCH 4/5] refactor(x): skills declare their own availability; excludeIds dies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Catalog membership for connection-gated skills (composio-integration, code-with-agents, slack) was decided by hand-rolled excludeIds forks inside buildCopilotInstructions — a third place that had to know which skill depends on which connection. Now each entry declares an availability() check (repos resolved via the new lazyResolve helper, so the skills module keeps no static DI edge; failures read as unavailable, the historical default), buildAvailableSkillCatalog evaluates them concurrently for the system prompt, and the excludeIds mechanism is deleted. Availability gates catalog VISIBILITY only — loadSkill still resolves explicitly-requested ids, exactly matching the old behavior. Tests cover the pure filter and pin that gated ids remain resolvable. Co-Authored-By: Claude Fable 5 --- .../assistant/capabilities/types.ts | 5 ++ .../src/application/assistant/instructions.ts | 14 ++-- .../assistant/skills/index.test.ts | 29 ++++++++ .../src/application/assistant/skills/index.ts | 74 ++++++++++++++++--- apps/x/packages/core/src/di/lazy-resolve.ts | 11 +++ 5 files changed, 114 insertions(+), 19 deletions(-) create mode 100644 apps/x/packages/core/src/di/lazy-resolve.ts 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 1472a428..f37035d4 100644 --- a/apps/x/packages/core/src/application/assistant/capabilities/types.ts +++ b/apps/x/packages/core/src/application/assistant/capabilities/types.ts @@ -70,6 +70,11 @@ export interface ModelCapability { content: string; // BuiltinTools keys this capability owns; attached mid-turn on load. tools?: string[]; + // Catalog visibility: when false, the capability is omitted from the + // loadSkill catalog (e.g. slack when no workspace is connected). + // Deliberately NOT a loadSkill fence — an explicitly-requested id still + // resolves, matching the historical excludeIds behavior. Defaults true. + availability?: () => Promise | boolean; } // App/always-activated: an eager system-prompt contribution composed at diff --git a/apps/x/packages/core/src/application/assistant/instructions.ts b/apps/x/packages/core/src/application/assistant/instructions.ts index f4009352..d5fb4953 100644 --- a/apps/x/packages/core/src/application/assistant/instructions.ts +++ b/apps/x/packages/core/src/application/assistant/instructions.ts @@ -1,4 +1,4 @@ -import { skillCatalog, buildSkillCatalog } from "./skills/index.js"; +import { skillCatalog, buildAvailableSkillCatalog } from "./skills/index.js"; import { getRuntimeContext, getRuntimeContextPrompt } from "./runtime-context.js"; import { composioAccountsRepo } from "../../composio/repo.js"; import { isConfigured as isComposioConfigured } from "../../composio/client.js"; @@ -409,13 +409,11 @@ export async function buildCopilotInstructions(): Promise { // knowledge sources unavailable — fall back to no channel hint } } - const excludeIds: string[] = []; - if (!composioEnabled) excludeIds.push('composio-integration'); - if (!codeModeEnabled) excludeIds.push('code-with-agents'); - if (!slackConnected) excludeIds.push('slack'); - // Always build from the live skill set so disk skills added/removed at - // runtime (after refreshDiskSkills + cache invalidation) are reflected. - const catalog = buildSkillCatalog({ excludeIds }); + // Catalog membership is each skill's own availability() (connection + // gates declared on the entries in skills/index.ts), evaluated against + // the live skill set so disk skills added/removed at runtime (after + // refreshDiskSkills + cache invalidation) are reflected. + const catalog = await buildAvailableSkillCatalog(); const baseInstructions = buildStaticInstructions(composioEnabled, catalog, codeModeEnabled, slackConnected, slackChannelsHint, googleConnected); const composioPrompt = await getComposioToolsPrompt(slackConnected, googleConnected); cachedInstructions = composioPrompt 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 48784e27..71056630 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 @@ -200,3 +200,32 @@ describe("capability activation boundary", () => { } }); }); + +describe("availability (catalog visibility)", () => { + it("drops unavailable entries from the catalog but keeps ungated ones", async () => { + const { filterAvailableEntries, buildCatalogFromEntries } = await import("./index.js"); + const entries = [ + { id: "always-on", title: "Always", summary: "s", content: "c" }, + { id: "connected", title: "Connected", summary: "s", content: "c", availability: () => true }, + { id: "disconnected", title: "Disconnected", summary: "s", content: "c", availability: async () => false }, + ]; + const available = await filterAvailableEntries(entries); + expect(available.map((e) => e.id)).toEqual(["always-on", "connected"]); + + const catalog = buildCatalogFromEntries(available); + expect(catalog).toContain("always-on"); + expect(catalog).toContain("connected"); + expect(catalog).not.toContain("disconnected"); + }); + + it("the connection-gated bundled skills declare availability", async () => { + const skills = await import("./index.js"); + const catalog = skills.buildSkillCatalog(); + // The ungated builder still lists them (loadSkill resolves explicit ids + // regardless of availability — visibility gating is catalog-only). + for (const id of ["composio-integration", "code-with-agents", "slack"]) { + expect(catalog).toContain(id); + expect(skills.resolveSkill(id)).not.toBeNull(); + } + }); +}); 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 bcbfc5e2..d21c7d47 100644 --- a/apps/x/packages/core/src/application/assistant/skills/index.ts +++ b/apps/x/packages/core/src/application/assistant/skills/index.ts @@ -104,6 +104,7 @@ const definitions: SkillDefinition[] = [ }, { id: "composio-integration", + availability: isComposioAvailable, title: "Composio Integration", summary: "Interact with third-party services (Gmail, GitHub, Slack, LinkedIn, Notion, Jira, Google Sheets, etc.) via Composio. Search, connect, and execute tools.", content: composioIntegrationSkill, @@ -131,6 +132,7 @@ const definitions: SkillDefinition[] = [ }, { id: "code-with-agents", + availability: isCodeModeAvailable, title: "Code with Agents", summary: "Write code, build projects, create scripts, or fix bugs by delegating to Claude Code or Codex.", content: codeWithAgentsSkill, @@ -178,9 +180,8 @@ const definitions: SkillDefinition[] = [ tools: ["browser-control", "load-browser-skill"], }, { - // Excluded from the catalog when Slack isn't connected (see - // buildCopilotInstructions excludeIds), mirroring composio-integration. id: "slack", + availability: isSlackAvailable, title: "Slack", summary: "Read, search, and send Slack messages via the agent-slack CLI — catch up on channels, summarize threads, list users. Load FIRST for ANY Slack request; Slack is connected natively, never through Composio.", content: slackSkill, @@ -237,12 +238,67 @@ function getSkillEntries(): SkillEntry[] { return [...bundledEntries, ...diskEntries]; } +// ---- Availability checks (catalog visibility) ---- +// Connection-state gates for the catalog, evaluated per build. Repos resolve +// lazily so this module keeps no static edge into the DI graph; any failure +// reads as "not available" (the historical default). + +async function isComposioAvailable(): Promise { + try { + const { isConfigured } = await import("../../../composio/client.js"); + return await isConfigured(); + } catch { + return false; + } +} + +async function isCodeModeAvailable(): Promise { + try { + const { lazyResolve } = await import("../../../di/lazy-resolve.js"); + const repo = await lazyResolve("codeModeConfigRepo"); + return (await repo.getConfig()).enabled; + } catch { + return false; + } +} + +async function isSlackAvailable(): Promise { + try { + const { lazyResolve } = await import("../../../di/lazy-resolve.js"); + const repo = await lazyResolve("slackConfigRepo"); + const config = await repo.getConfig(); + return config.enabled && config.workspaces.length > 0; + } catch { + return false; + } +} + +// Pure availability filter over an explicit entry list (exported for tests): +// entries without an availability check are kept; checks are evaluated +// concurrently and a false drops the entry from the CATALOG only (loadSkill +// still resolves explicit ids, matching the historical excludeIds behavior). +export async function filterAvailableEntries Promise | boolean }>( + entries: readonly T[], +): Promise { + const verdicts = await Promise.all( + entries.map((entry) => (entry.availability ? entry.availability() : true)), + ); + return entries.filter((_, i) => verdicts[i]); +} + +// The catalog restricted to currently-available skills — what +// buildCopilotInstructions embeds in the system prompt. +export async function buildAvailableSkillCatalog(): Promise { + return buildCatalogFromEntries(await filterAvailableEntries(getSkillEntries())); +} + /** - * Build a skill catalog string, optionally excluding specific skills by ID. - * Reads the live skill set, so it reflects disk skills added/removed at runtime. + * Build a skill catalog string from the live skill set (availability + * ignored), so it reflects disk skills added/removed at runtime. The + * system prompt uses buildAvailableSkillCatalog instead. */ -export function buildSkillCatalog(options?: { excludeIds?: string[] }): string { - return buildCatalogFromEntries(getSkillEntries(), options); +export function buildSkillCatalog(): string { + return buildCatalogFromEntries(getSkillEntries()); } // Pure catalog builder over an explicit entry list (exported for the @@ -251,15 +307,11 @@ export function buildSkillCatalog(options?: { excludeIds?: string[] }): string { // 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( + const entries = 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 ?? `${CATALOG_PREFIX}/${entry.id}/skill.ts`}\``, diff --git a/apps/x/packages/core/src/di/lazy-resolve.ts b/apps/x/packages/core/src/di/lazy-resolve.ts new file mode 100644 index 00000000..e3eb65fe --- /dev/null +++ b/apps/x/packages/core/src/di/lazy-resolve.ts @@ -0,0 +1,11 @@ +// Lazily import the DI container and resolve one token. Use this instead of +// a static `import container from "./container.js"` when the importing +// module must not add a static edge into the DI graph (the container's +// module tree is large, and some importers — the agent registry, tool +// handlers, capability availability checks — are themselves imported while +// the container initializes). One helper holds the pattern and this +// rationale instead of hand-rolled copies at every call site. +export async function lazyResolve(token: string): Promise { + const { default: container } = await import("./container.js"); + return container.resolve(token); +} From ca077faa95baca1dbfd416767541c1baf42bebf4 Mon Sep 17 00:00:00 2001 From: Ramnique Singh <30795890+ramnique@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:50:39 +0530 Subject: [PATCH 5/5] refactor(x): shared lazyResolve for the no-static-DI-edge pattern Four modules hand-rolled the same lazy container-import-and-resolve dance (agent registry, spawn-agent, background-task runner, notifier), each restating the rationale. di/lazy-resolve.ts holds the pattern and the reasoning once; all four sites migrate. Co-Authored-By: Claude Fable 5 --- apps/x/packages/core/src/agents/registry.ts | 7 +++---- apps/x/packages/core/src/agents/spawn-agent.ts | 6 +++--- .../packages/core/src/application/notification/notifier.ts | 4 ++-- apps/x/packages/core/src/background-tasks/runner.ts | 4 ++-- 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/apps/x/packages/core/src/agents/registry.ts b/apps/x/packages/core/src/agents/registry.ts index 9508ea7f..f09a5034 100644 --- a/apps/x/packages/core/src/agents/registry.ts +++ b/apps/x/packages/core/src/agents/registry.ts @@ -10,6 +10,7 @@ import { getRaw as getLabelingAgentRaw } from "../knowledge/labeling_agent.js"; import { getRaw as getNoteTaggingAgentRaw } from "../knowledge/note_tagging_agent.js"; import { getRaw as getInlineTaskAgentRaw } from "../knowledge/inline_task_agent.js"; import { getRaw as getAgentNotesAgentRaw } from "../knowledge/agent_notes_agent.js"; +import { lazyResolve } from "../di/lazy-resolve.js"; import type { IAgentsRepo } from "./repo.js"; // The registry of built-in agents: one table instead of the historical @@ -89,9 +90,7 @@ export async function loadAgent(id: string): Promise> { if (builtin) { return builtin.build(); } - // User-defined agents. The container is imported lazily so this module - // adds no static edge into the DI graph (mirrors spawn-agent). - const { default: container } = await import("../di/container.js"); - const repo = container.resolve("agentsRepo"); + // User-defined agents (lazyResolve: no static DI edge from this module). + const repo = await lazyResolve("agentsRepo"); return repo.fetch(id); } diff --git a/apps/x/packages/core/src/agents/spawn-agent.ts b/apps/x/packages/core/src/agents/spawn-agent.ts index 432edaab..72cbf26c 100644 --- a/apps/x/packages/core/src/agents/spawn-agent.ts +++ b/apps/x/packages/core/src/agents/spawn-agent.ts @@ -219,13 +219,13 @@ export async function runSpawnedAgent( async function resolveServices(): Promise< NonNullable > { - const { default: container } = await import("../di/container.js"); + const { lazyResolve } = await import("../di/lazy-resolve.js"); return { turnRuntime: - container.resolve( + await lazyResolve( "turnRuntime", ), - headlessRunner: container.resolve< + headlessRunner: await lazyResolve< import("./headless.js").IHeadlessAgentRunner >("headlessAgentRunner"), }; diff --git a/apps/x/packages/core/src/application/notification/notifier.ts b/apps/x/packages/core/src/application/notification/notifier.ts index a1cacc76..050279d3 100644 --- a/apps/x/packages/core/src/application/notification/notifier.ts +++ b/apps/x/packages/core/src/application/notification/notifier.ts @@ -19,8 +19,8 @@ export async function notifyIfEnabled( ): Promise { try { if (!isNotificationCategoryEnabled(category)) return; - const { default: container } = await import('../../di/container.js'); - const service = container.resolve('notificationService'); + const { lazyResolve } = await import('../../di/lazy-resolve.js'); + const service = await lazyResolve('notificationService'); if (!service.isSupported()) return; service.notify(input); } catch (err) { diff --git a/apps/x/packages/core/src/background-tasks/runner.ts b/apps/x/packages/core/src/background-tasks/runner.ts index 810b9aac..1463d27b 100644 --- a/apps/x/packages/core/src/background-tasks/runner.ts +++ b/apps/x/packages/core/src/background-tasks/runner.ts @@ -128,8 +128,8 @@ export async function runBackgroundTask( let codeProject: { id: string; path: string; name: string } | undefined; if (task.projectId) { try { - const { default: container } = await import('../di/container.js'); - const projectsRepo = container.resolve('codeProjectsRepo'); + const { lazyResolve } = await import('../di/lazy-resolve.js'); + const projectsRepo = await lazyResolve('codeProjectsRepo'); const project = await projectsRepo.get(task.projectId); if (project) codeProject = { id: project.id, path: project.path, name: project.name }; } catch (err) {