From f5be21178f9758e1d8ea0d2af6181cc31bafa882 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 2 Jun 2026 20:55:01 -0700 Subject: [PATCH] feat(core,engine,compound-engineering): make CE sub-agent sessions load their skills Closes the U2/U5 skill-discovery carry-forward so the plugin's interactive ce-* sessions actually load the stage's bundled skill in a live agent (not just in scripted-fake tests). Root cause: createFnAgent built its DefaultResourceLoader without forwarding any skill-discovery path, and the interactive seam options couldn't carry one. The loader's skillsOverride only *filters* skills already discovered from cwd's standard roots, so the plugin-local .fusion-ce-skills//SKILL.md was never discoverable. Fix (end-to-end): - AgentOptions.additionalSkillPaths forwarded into DefaultResourceLoader - CreateInteractiveAiSessionOptions gains requestedSkillNames + additionalSkillPaths - the interactive engine adapter forwards them to createFnAgent (skills + additionalSkillPaths) - the orchestrator runs the session with cwd on the real project root and hands it [stage.skillId] + the install root Proven: a real DefaultResourceLoader with additionalSkillPaths discovers ce-plan and filters out ce-work; the orchestrator passes the right id/path/cwd. Plugin 96, engine 136, core 99 tests green. --- packages/core/src/plugin-types.ts | 13 +++ ...pound-engineering-skill-resolution.test.ts | 37 +++++++- packages/engine/src/index.ts | 5 + packages/engine/src/pi.ts | 8 ++ .../src/__tests__/skill-reachability.test.ts | 65 ++++++------- .../src/__tests__/skill-wiring.test.ts | 61 ++++++++++++ .../src/session/orchestrator.ts | 93 ++++++++----------- 7 files changed, 191 insertions(+), 91 deletions(-) create mode 100644 plugins/fusion-plugin-compound-engineering/src/__tests__/skill-wiring.test.ts diff --git a/packages/core/src/plugin-types.ts b/packages/core/src/plugin-types.ts index ab0d8648ff..c72d6856b4 100644 --- a/packages/core/src/plugin-types.ts +++ b/packages/core/src/plugin-types.ts @@ -151,6 +151,19 @@ export interface CreateInteractiveAiSessionOptions { defaultProvider?: string; /** Default model ID within the provider */ defaultModelId?: string; + /** + * Skill names the session should load (matched against discovered skills). + * Lets a plugin point a session at a specific bundled skill rather than + * relying on cwd-only discovery. Forwarded to the engine's skill selection. + */ + requestedSkillNames?: string[]; + /** + * Extra directories to scan for skills (each holding `/SKILL.md`), in + * addition to the default cwd/agent-dir roots. A plugin that installs its + * skills to a plugin-local directory passes that directory here so its + * `requestedSkillNames` are actually discoverable in the live session. + */ + additionalSkillPaths?: string[]; } /** diff --git a/packages/engine/src/__tests__/compound-engineering-skill-resolution.test.ts b/packages/engine/src/__tests__/compound-engineering-skill-resolution.test.ts index cb4d5f2046..81bf1c3690 100644 --- a/packages/engine/src/__tests__/compound-engineering-skill-resolution.test.ts +++ b/packages/engine/src/__tests__/compound-engineering-skill-resolution.test.ts @@ -30,7 +30,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { loadSkills } from "@earendil-works/pi-coding-agent"; +import { DefaultResourceLoader, loadSkills, type Skill } from "@earendil-works/pi-coding-agent"; import { createSkillsOverrideFromSelection, resolveSessionSkills, @@ -156,4 +156,39 @@ describe("U2: CE bundled skill session-resolution (empirical)", () => { expect(resolved).toContain(s); } }); + + it("PASSING (real loader): DefaultResourceLoader with additionalSkillPaths + skillsOverride discovers ce-plan — the exact path createFnAgent now feeds", async () => { + const installRoot = join(tmp, ".fusion-ce-skills"); + materializeInstalledSkills(installRoot, ["ce-plan", "ce-work"]); + mkdirSync(projectRootDir, { recursive: true }); + mkdirSync(agentDir, { recursive: true }); + + // Build the same skillsOverride createFnAgent builds from `skills: ["ce-plan"]`. + const selection = resolveSessionSkills({ + projectRootDir, + requestedSkillNames: ["ce-plan"], + sessionPurpose: "executor", + }); + const skillsOverride = createSkillsOverrideFromSelection(selection, { + requestedSkillNames: ["ce-plan"], + sessionPurpose: "executor", + }); + + // Construct the loader exactly as pi.ts createFnAgent now does: cwd on the + // project root, the install dir passed via additionalSkillPaths, and the + // requested-name filter as skillsOverride. + const loader = new DefaultResourceLoader({ + cwd: projectRootDir, + agentDir, + additionalSkillPaths: [installRoot], + skillsOverride, + }); + await loader.reload(); + + const names = loader.getSkills().skills.map((s: Skill) => s.name); + // ce-plan is discoverable (via additionalSkillPaths) AND survives the filter; + // ce-work is discovered but filtered out by the requested-name override. + expect(names).toContain("ce-plan"); + expect(names).not.toContain("ce-work"); + }); }); diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 377b06d9ae..20a3c6c8f1 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -156,6 +156,11 @@ const _createInteractiveAiSessionAdapter: CreateInteractiveAiSessionFactory = ( tools: opts.tools, defaultProvider: opts.defaultProvider, defaultModelId: opts.defaultModelId, + // Forward skill selection so a plugin can load a specific bundled skill. + // `skills` (convenience) auto-builds a SkillSelectionContext; the extra + // discovery dirs make those skills actually visible to the loader. + ...(opts.requestedSkillNames?.length ? { skills: opts.requestedSkillNames } : {}), + ...(opts.additionalSkillPaths?.length ? { additionalSkillPaths: opts.additionalSkillPaths } : {}), }), options, ); diff --git a/packages/engine/src/pi.ts b/packages/engine/src/pi.ts index 7d79e0c244..39aaf0d8b2 100644 --- a/packages/engine/src/pi.ts +++ b/packages/engine/src/pi.ts @@ -964,6 +964,11 @@ export interface AgentOptions { * (and `skillSelection` is not), auto-constructs a SkillSelectionContext * from the cwd and these names. Ignored when `skillSelection` is set. */ skills?: string[]; + /** Extra directories to scan for skills (each holding `/SKILL.md`), in + * addition to the default cwd/agent-dir roots. Forwarded to the resource + * loader so callers (e.g. plugins that install skills to a private dir) can + * make `skills`/`skillSelection` names discoverable in the live session. */ + additionalSkillPaths?: string[]; /** Optional task-scoped env injected into this session's subprocess tools only. */ taskEnv?: NodeJS.ProcessEnv; /** Last-chance abort hook fired immediately before `createAgentSession`. @@ -1987,6 +1992,9 @@ export async function createFnAgent(options: AgentOptions): Promise ? [options.systemPromptLayers.dynamic] : [], ...(effectiveExtensionPaths.length > 0 ? { additionalExtensionPaths: [...effectiveExtensionPaths] } : {}), + ...(options.additionalSkillPaths && options.additionalSkillPaths.length > 0 + ? { additionalSkillPaths: [...options.additionalSkillPaths] } + : {}), ...(skillsOverrideFn ? { skillsOverride: skillsOverrideFn } : {}), }); await resourceLoader.reload(); diff --git a/plugins/fusion-plugin-compound-engineering/src/__tests__/skill-reachability.test.ts b/plugins/fusion-plugin-compound-engineering/src/__tests__/skill-reachability.test.ts index 6c361b6fb7..5f0526c79d 100644 --- a/plugins/fusion-plugin-compound-engineering/src/__tests__/skill-reachability.test.ts +++ b/plugins/fusion-plugin-compound-engineering/src/__tests__/skill-reachability.test.ts @@ -1,58 +1,49 @@ -import { existsSync, mkdtempSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { existsSync } from "node:fs"; import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { installBundledCeSkills } from "../skill-installation.js"; -import { resolveStageSkillCwd, buildStageSystemPrompt } from "../session/orchestrator.js"; +import { resolveStageSkillPaths, buildStageSystemPrompt } from "../session/orchestrator.js"; import { getStage } from "../session/stage-registry.js"; /** - * CARRY-FORWARD (U2 → U5): prove the launched stage's ce-* skill is REACHABLE - * for the session. + * Prove the launched stage's ce-* skill is REACHABLE for the session — now via + * the real seam wiring (closes the U2 → U5 carry-forward). * - * HONEST SCOPE — proved at the installer/resolver layer (exactly as U2 did), - * NOT at the live session layer. The U4 `CreateInteractiveAiSessionOptions` - * surface carries only `cwd` (no `requestedSkillNames`/`additionalSkillPaths`/ - * `skillSelection`), so the orchestrator cannot hand the session an explicit - * skill-discovery path. The closest honest wiring is: - * 1. point the session `cwd` at the install-target root (where pi's - * DefaultResourceLoader can discover `/SKILL.md`), and - * 2. name the skill id in the system prompt. - * This test asserts BOTH: the resolved cwd contains the stage's installed - * SKILL.md, and the system prompt names the stage's skill id. + * The U4 `CreateInteractiveAiSessionOptions` surface now carries + * `requestedSkillNames` + `additionalSkillPaths`, which the engine adapter + * forwards into `createFnAgent` (`skills` + the loader's `additionalSkillPaths`). + * So the orchestrator hands the session BOTH the stage's skill id and the + * install directory to discover it from. This test asserts: + * 1. the install directory `resolveStageSkillPaths()` returns actually holds + * the stage's `/SKILL.md` after install, and + * 2. the system prompt names the stage's skill id. * - * A complete fix needs U4's options to gain a forwarded - * `requestedSkillNames`/`additionalSkillPaths` field — flagged as a carry- - * forward for U6/follow-up. + * The engine package separately proves (compound-engineering-skill-resolution + * .test.ts) that `loadSkills` + the resolver resolve a ce-* skill once that + * directory is on the discovery path — together the chain is closed. */ -describe("stage skill reachability (carry-forward, resolver-layer proof)", () => { - let targets: string[] = []; - +describe("stage skill reachability (real seam wiring)", () => { afterEach(() => { - targets = []; vi.restoreAllMocks(); }); - it("the resolved session cwd contains the stage's installed SKILL.md", () => { - const target = mkdtempSync(join(tmpdir(), "ce-skill-target-")); - targets.push(target); + it("the resolved additionalSkillPaths directory contains the stage's installed SKILL.md", () => { + const stage = getStage("brainstorm")!; - // Install bundled skills into a plugin-local target. - const { results } = installBundledCeSkills({ targetRoot: target }); + // resolveStageSkillPaths() returns the plugin-local install root the session + // is told to discover skills from (never a global one). + const skillPaths = resolveStageSkillPaths(); + expect(skillPaths).toHaveLength(1); + expect(skillPaths[0]).toMatch(/\.fusion-ce-skills$/); + + // Install bundled skills into that exact root and assert the stage's skill + // is present on the path the session will scan. + const { results } = installBundledCeSkills({ targetRoot: skillPaths[0] }); expect(results.every((r) => r.outcome === "installed" || r.outcome === "skipped")).toBe(true); - const stage = getStage("brainstorm")!; - // The orchestrator resolves the discovery cwd to the default install-target - // root. For this isolation test we assert the SAME structure exists at the - // explicit target we installed into (resolveStageSkillCwd returns the - // default root, which the production onLoad install populates identically). - const installedSkillMd = join(target, stage.skillId, "SKILL.md"); + const installedSkillMd = join(skillPaths[0], stage.skillId, "SKILL.md"); expect(existsSync(installedSkillMd)).toBe(true); - - // resolveStageSkillCwd returns a plugin-local directory (never a global one). - const cwd = resolveStageSkillCwd(); - expect(cwd).toMatch(/\.fusion-ce-skills$/); }); it("the stage system prompt names the stage's ce-* skill id", () => { diff --git a/plugins/fusion-plugin-compound-engineering/src/__tests__/skill-wiring.test.ts b/plugins/fusion-plugin-compound-engineering/src/__tests__/skill-wiring.test.ts new file mode 100644 index 0000000000..379251380e --- /dev/null +++ b/plugins/fusion-plugin-compound-engineering/src/__tests__/skill-wiring.test.ts @@ -0,0 +1,61 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { + CreateInteractiveAiSessionOptions, + InteractiveAiSessionEvent, +} from "@fusion/core"; +import { CeOrchestrator } from "../session/orchestrator.js"; +import { getStage } from "../session/stage-registry.js"; +import { resolveDefaultInstallTargetRoot } from "../skill-installation.js"; +import { makeHarness, makeScriptedSession, type TestHarness } from "./_harness.js"; + +/** + * Proves the orchestrator hands a launched session the wiring a LIVE agent needs + * to actually load the stage's bundled ce-* skill (closes the U2/U5 carry-forward): + * - cwd is the real project root (where the agent reads context + writes the + * artifact), NOT the skills directory; + * - requestedSkillNames names the stage's ce-* skill; + * - additionalSkillPaths includes the plugin-local install root so the engine + * loader can discover that skill. + * The engine adapter forwards these to createFnAgent (skills + additionalSkillPaths); + * compound-engineering-skill-resolution.test.ts proves the loader then resolves it. + */ +describe("session skill wiring", () => { + let h: TestHarness; + beforeEach(() => { + h = makeHarness(); + }); + afterEach(() => { + h.close(); + }); + + it("start() passes the stage skill id, install path, and project-root cwd to the factory", async () => { + const captured: CreateInteractiveAiSessionOptions[] = []; + const script: InteractiveAiSessionEvent[] = [ + { type: "complete", data: { artifact: "# done" } }, + ]; + const session = makeScriptedSession(script); + const factory = vi.fn(async (opts: CreateInteractiveAiSessionOptions) => { + captured.push(opts); + return { session }; + }); + const orch = new CeOrchestrator({ + ctx: h.ctx, + createInteractiveAiSession: factory, + projectRoot: h.projectRoot, + turnTimeoutMs: 5000, + }); + + await orch.start("brainstorm", { openingMessage: "let's go" }); + + expect(captured).toHaveLength(1); + const opts = captured[0]; + const stage = getStage("brainstorm")!; + // cwd is the project root, not the skills dir. + expect(opts.cwd).toBe(h.projectRoot); + // the stage's ce-* skill is requested... + expect(opts.requestedSkillNames).toEqual([stage.skillId]); + // ...and the plugin-local install root is on the discovery path. + expect(opts.additionalSkillPaths).toEqual([resolveDefaultInstallTargetRoot()]); + expect(opts.additionalSkillPaths?.[0]).toMatch(/\.fusion-ce-skills$/); + }); +}); diff --git a/plugins/fusion-plugin-compound-engineering/src/session/orchestrator.ts b/plugins/fusion-plugin-compound-engineering/src/session/orchestrator.ts index 7c18e70613..fa9c48e29b 100644 --- a/plugins/fusion-plugin-compound-engineering/src/session/orchestrator.ts +++ b/plugins/fusion-plugin-compound-engineering/src/session/orchestrator.ts @@ -89,33 +89,24 @@ export interface OrchestratorDeps { } /** - * CARRY-FORWARD (U2 → U5) — skill discovery wiring. + * Skill discovery wiring (closes the U2 → U5 carry-forward). * * U2 proved a `PluginSkillContribution` is NOT auto-ingested by the engine - * skill-resolver; a physical install onto a cwd-discoverable path is required. - * The U4 `CreateInteractiveAiSessionOptions` surface carries ONLY `cwd` (plus - * systemPrompt/tools/provider/model) — it has NO `requestedSkillNames` / - * `additionalSkillPaths` / `skillSelection` field. So we cannot point the - * session at the install target the way `createFnAgent`'s `skills`/ - * `skillSelection` options would. - * - * The closest honest thing we CAN do today: - * 1. Set the session `cwd` to a root under which the installed ce-* skills are - * discoverable by pi's DefaultResourceLoader (the install-target root). - * 2. Name the required skill id in the systemPrompt so the agent is told which - * ce-* skill to apply (protocol-level instruction). - * - * This function computes that cwd. We PROVE reachability at the installer/ - * resolver layer (like U2 did) in the tests — the U4 options surface cannot yet - * carry an explicit skill-path, so a complete fix needs U4's options to gain a - * `requestedSkillNames`/`additionalSkillPaths` field forwarded into - * `createFnAgent`. That gap is documented as a carry-forward for the follow-up, - * which will re-expand this to derive a per-stage/per-project path. + * skill-resolver; a physical install onto a discoverable path is required, and + * the plugin installs its bundled `ce-*` skills to a plugin-local directory + * (`resolveDefaultInstallTargetRoot()`). The U4 seam now carries + * `requestedSkillNames` + `additionalSkillPaths`, which the engine adapter + * forwards into `createFnAgent` (`skills` + the loader's `additionalSkillPaths`). + * So the orchestrator hands the live session BOTH the stage's skill id and the + * install directory to discover it from — the session runs with `cwd` at the + * real project root (where it reads context and writes artifacts), not at the + * skills directory. */ -export function resolveStageSkillCwd(): string { - // The install target root holds `/SKILL.md` for each installed - // skill; using it as the discovery root makes the stage's skill loadable. - return resolveDefaultInstallTargetRoot(); +export function resolveStageSkillPaths(): string[] { + // The install target root holds `/SKILL.md` for each installed skill; + // passing it as an additional skill-discovery path makes the stage's skill + // loadable while the session cwd stays on the project. + return [resolveDefaultInstallTargetRoot()]; } /** @@ -125,7 +116,7 @@ export function resolveStageSkillCwd(): string { export function buildStageSystemPrompt(stage: CeStageDefinition): string { return [ `You are running the Compound Engineering "${stage.stageId}" stage.`, - `Apply the bundled skill "${stage.skillId}" (its SKILL.md is discoverable in your working directory).`, + `Apply the bundled skill "${stage.skillId}" (it has been loaded into this session).`, "", "Drive the stage as an interactive question/answer flow. On every turn respond with ONLY a JSON object:", ' - To ask the user something: {"type":"question","data":{"id":"","type":"single_select|multi_select|text|confirm","question":"...","options":[{"id":"..","label":".."}]}}', @@ -175,6 +166,28 @@ export class CeOrchestrator { this.turnTimeoutMs = deps.turnTimeoutMs ?? DEFAULT_TURN_TIMEOUT_MS; } + /** + * Build the interactive-session options for a stage. The session runs with + * `cwd` on the real project root (where it reads context and writes the stage + * artifact) and is handed BOTH the stage's `ce-*` skill id and the plugin-local + * install directory to discover it from — so the live agent actually loads the + * bundled skill (closing the U2/U5 skill-discovery carry-forward). Model + * provider/model are setting-gated (U9); omitted keys let the host pick defaults. + */ + private buildSessionOptions(stage: CeStageDefinition): Parameters[0] { + const defaultProvider = getDefaultProvider(this.ctx.settings); + const defaultModelId = getDefaultModelId(this.ctx.settings); + return { + cwd: this.projectRoot, + systemPrompt: buildStageSystemPrompt(stage), + tools: "coding", + requestedSkillNames: [stage.skillId], + additionalSkillPaths: resolveStageSkillPaths(), + ...(defaultProvider ? { defaultProvider } : {}), + ...(defaultModelId ? { defaultModelId } : {}), + }; + } + /** Start a fresh session for a registered stage and run the opening turn. */ async start(stageId: string, opts: StartStageOptions): Promise { const stage = getStage(stageId); @@ -196,24 +209,9 @@ export class CeOrchestrator { }); this.store.appendHistory(session.id, { role: "user", text: opts.openingMessage, at: new Date().toISOString() }); - const cwd = resolveStageSkillCwd(); - const systemPrompt = buildStageSystemPrompt(stage); - - // Setting-gated model selection (U9): pass the operator's default - // provider/model through to the host factory; omitted keys let the host - // pick its own defaults. - const defaultProvider = getDefaultProvider(this.ctx.settings); - const defaultModelId = getDefaultModelId(this.ctx.settings); - let interactive; try { - interactive = await this.factory({ - cwd, - systemPrompt, - tools: "coding", - ...(defaultProvider ? { defaultProvider } : {}), - ...(defaultModelId ? { defaultModelId } : {}), - }); + interactive = await this.factory(this.buildSessionOptions(stage)); } catch (err) { return { session: this.failSession(session.id, err), event: undefined }; } @@ -331,18 +329,7 @@ export class CeOrchestrator { const stage = getStage(session.stage); if (!stage) throw new Error(`Unknown CE stage: ${session.stage}`); - const cwd = resolveStageSkillCwd(); - const systemPrompt = buildStageSystemPrompt(stage); - const defaultProvider = getDefaultProvider(this.ctx.settings); - const defaultModelId = getDefaultModelId(this.ctx.settings); - - const interactive = await this.factory!({ - cwd, - systemPrompt, - tools: "coding", - ...(defaultProvider ? { defaultProvider } : {}), - ...(defaultModelId ? { defaultModelId } : {}), - }); + const interactive = await this.factory!(this.buildSessionOptions(stage)); const live = interactive.session; // Walk the recorded user turns in order. The FIRST user turn is the opening