From 2844d6ece89d3b3a2d0c096a5e6b1ab203bb3616 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sun, 11 Jan 2026 03:07:28 +0100 Subject: [PATCH] refactor(discovery): extracted skill loading logic into reusable helper to eliminate duplication - Extracted skill loading logic into reusable `loadSkillsFromDir` helper function to eliminate code duplication across builtin, claude, and codex discovery providers. - Refactored builtin provider to use new `loadSkillsFromDir` helper instead of inline `loadSkillsRecursive` function. - Refactored claude provider to use new `loadSkillsFromDir` helper, reducing function from 78 lines to 25 lines. - Refactored codex provider to use new `loadSkillsFromDir` helper, consolidating duplicate skill transformation and frontmatter parsing logic. - Updated extension module discovery to skip `node_modules` directories in addition to hidden directories. - Added guideline to Task tool documentation about isolating file scopes for concurrent agents to prevent conflicts. --- packages/coding-agent/CHANGELOG.md | 5 +- .../coding-agent/src/discovery/builtin.ts | 63 ++------------ packages/coding-agent/src/discovery/claude.ts | 85 ++++--------------- packages/coding-agent/src/discovery/codex.ts | 47 +++------- .../coding-agent/src/discovery/helpers.ts | 53 +++++++++++- .../coding-agent/src/prompts/tools/task.md | 1 + 6 files changed, 93 insertions(+), 161 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b8a89fe55..c323e0326 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,7 +1,6 @@ # Changelog ## [Unreleased] - ### Added - Added automatic discovery and listing of AGENTS.md files in the system prompt, providing agents with an authoritative list of project-specific instruction files without runtime searching @@ -9,10 +8,14 @@ ### Changed +- Refactored skill discovery to use unified `loadSkillsFromDir` helper across all providers, reducing code duplication +- Updated skill discovery to scan only `skills/*/SKILL.md` entries instead of recursive walks in Codex provider +- Added guidance to Task tool documentation to isolate file scopes when assigning tasks to prevent agent conflicts - Updated Task tool documentation to emphasize that subagents have no access to conversation history and require all relevant context to be explicitly passed - Revised task agent prompt to clarify that subagents have full tool access and can make file edits, run commands, and create files - OpenAI Codex: updated to use bundled system prompt from upstream - Changed `complete` tool to make `data` parameter optional when aborting, while still requiring it for successful completions +- Skills discovery now scans only `skills/*/SKILL.md` entries instead of recursive walks ### Removed diff --git a/packages/coding-agent/src/discovery/builtin.ts b/packages/coding-agent/src/discovery/builtin.ts index a7942337b..82ba8f1c6 100644 --- a/packages/coding-agent/src/discovery/builtin.ts +++ b/packages/coding-agent/src/discovery/builtin.ts @@ -5,7 +5,7 @@ * .pi is an alias for backwards compatibility. */ -import { basename, dirname, isAbsolute, join, resolve } from "path"; +import { dirname, isAbsolute, join, resolve } from "path"; import { type ContextFile, contextFileCapability } from "../capability/context-file"; import { type Extension, type ExtensionManifest, extensionCapability } from "../capability/extension"; import { type ExtensionModule, extensionModuleCapability } from "../capability/extension-module"; @@ -16,7 +16,7 @@ import { type MCPServer, mcpCapability } from "../capability/mcp"; import { type Prompt, promptCapability } from "../capability/prompt"; import { type Rule, ruleCapability } from "../capability/rule"; import { type Settings, settingsCapability } from "../capability/settings"; -import { type Skill, type SkillFrontmatter, skillCapability } from "../capability/skill"; +import { type Skill, skillCapability } from "../capability/skill"; import { type SlashCommand, slashCommandCapability } from "../capability/slash-command"; import { type SystemPrompt, systemPromptCapability } from "../capability/system-prompt"; import { type CustomTool, toolCapability } from "../capability/tool"; @@ -27,6 +27,7 @@ import { expandEnvVarsDeep, getExtensionNameFromPath, loadFilesFromDir, + loadSkillsFromDir, parseFrontmatter, parseJSON, SOURCE_PATHS, @@ -190,64 +191,18 @@ registerProvider(systemPromptCapability.id, { }); // Skills -function loadSkillFromFile(ctx: LoadContext, path: string, level: "user" | "project"): Skill | null { - const content = ctx.fs.readFile(path); - if (!content) return null; - - const { frontmatter, body } = parseFrontmatter(content); - const skillDir = dirname(path); - const parentDirName = basename(skillDir); - const name = (frontmatter.name as string) || parentDirName; - - if (!frontmatter.description) return null; - - return { - name, - path, - content: body, - frontmatter: frontmatter as SkillFrontmatter, - level, - _source: createSourceMeta(PROVIDER_ID, path, level), - }; -} - -function loadSkillsRecursive(ctx: LoadContext, dir: string, level: "user" | "project"): LoadResult { - const items: Skill[] = []; - const warnings: string[] = []; - - if (!ctx.fs.isDir(dir)) return { items, warnings }; - - for (const name of ctx.fs.readDir(dir)) { - if (name.startsWith(".") || name === "node_modules") continue; - - const path = join(dir, name); - - if (ctx.fs.isDir(path)) { - const skillFile = join(path, "SKILL.md"); - if (ctx.fs.isFile(skillFile)) { - const skill = loadSkillFromFile(ctx, skillFile, level); - if (skill) items.push(skill); - } - - const sub = loadSkillsRecursive(ctx, path, level); - items.push(...sub.items); - if (sub.warnings) warnings.push(...sub.warnings); - } else if (name === "SKILL.md") { - const skill = loadSkillFromFile(ctx, path, level); - if (skill) items.push(skill); - } - } - - return { items, warnings }; -} - function loadSkills(ctx: LoadContext): LoadResult { const items: Skill[] = []; const warnings: string[] = []; for (const { dir, level } of getConfigDirs(ctx)) { const skillsDir = join(dir, "skills"); - const result = loadSkillsRecursive(ctx, skillsDir, level); + const result = loadSkillsFromDir(ctx, { + dir: skillsDir, + providerId: PROVIDER_ID, + level, + requireDescription: true, + }); items.push(...result.items); if (result.warnings) warnings.push(...result.warnings); } diff --git a/packages/coding-agent/src/discovery/claude.ts b/packages/coding-agent/src/discovery/claude.ts index 85aeb7397..35e3e5652 100644 --- a/packages/coding-agent/src/discovery/claude.ts +++ b/packages/coding-agent/src/discovery/claude.ts @@ -24,7 +24,7 @@ import { expandEnvVarsDeep, getExtensionNameFromPath, loadFilesFromDir, - parseFrontmatter, + loadSkillsFromDir, parseJSON, } from "./helpers"; @@ -218,78 +218,25 @@ function loadSkills(ctx: LoadContext): LoadResult { const items: Skill[] = []; const warnings: string[] = []; - // User-level: ~/.claude/skills/*/SKILL.md - const userBase = getUserClaude(ctx); - const userSkillsDir = join(userBase, "skills"); + const userSkillsDir = join(getUserClaude(ctx), "skills"); + const userResult = loadSkillsFromDir(ctx, { + dir: userSkillsDir, + providerId: PROVIDER_ID, + level: "user", + }); + items.push(...userResult.items); + if (userResult.warnings) warnings.push(...userResult.warnings); - if (ctx.fs.isDir(userSkillsDir)) { - const skillDirs = ctx.fs.readDir(userSkillsDir); - - for (const dirName of skillDirs) { - if (dirName.startsWith(".")) continue; - - const skillDir = join(userSkillsDir, dirName); - if (!ctx.fs.isDir(skillDir)) continue; - - const skillFile = join(skillDir, "SKILL.md"); - if (!ctx.fs.isFile(skillFile)) continue; - - const content = ctx.fs.readFile(skillFile); - if (!content) { - warnings.push(`Failed to read ${skillFile}`); - continue; - } - - const { frontmatter, body } = parseFrontmatter(content); - const name = (frontmatter.name as string) || dirName; - - items.push({ - name, - path: skillFile, - content: body, - frontmatter, - level: "user", - _source: createSourceMeta(PROVIDER_ID, skillFile, "user"), - }); - } - } - - // Project-level: /.claude/skills/*/SKILL.md const projectBase = getProjectClaude(ctx); if (projectBase) { const projectSkillsDir = join(projectBase, "skills"); - - if (ctx.fs.isDir(projectSkillsDir)) { - const skillDirs = ctx.fs.readDir(projectSkillsDir); - - for (const dirName of skillDirs) { - if (dirName.startsWith(".")) continue; - - const skillDir = join(projectSkillsDir, dirName); - if (!ctx.fs.isDir(skillDir)) continue; - - const skillFile = join(skillDir, "SKILL.md"); - if (!ctx.fs.isFile(skillFile)) continue; - - const content = ctx.fs.readFile(skillFile); - if (!content) { - warnings.push(`Failed to read ${skillFile}`); - continue; - } - - const { frontmatter, body } = parseFrontmatter(content); - const name = (frontmatter.name as string) || dirName; - - items.push({ - name, - path: skillFile, - content: body, - frontmatter, - level: "project", - _source: createSourceMeta(PROVIDER_ID, skillFile, "project"), - }); - } - } + const projectResult = loadSkillsFromDir(ctx, { + dir: projectSkillsDir, + providerId: PROVIDER_ID, + level: "project", + }); + items.push(...projectResult.items); + if (projectResult.warnings) warnings.push(...projectResult.warnings); } return { items, warnings }; diff --git a/packages/coding-agent/src/discovery/codex.ts b/packages/coding-agent/src/discovery/codex.ts index 383899493..f7c53fe1c 100644 --- a/packages/coding-agent/src/discovery/codex.ts +++ b/packages/coding-agent/src/discovery/codex.ts @@ -33,6 +33,7 @@ import { discoverExtensionModulePaths, getExtensionNameFromPath, loadFilesFromDir, + loadSkillsFromDir, parseFrontmatter, SOURCE_PATHS, } from "./helpers"; @@ -209,51 +210,25 @@ function loadSkills(ctx: LoadContext): LoadResult { const items: Skill[] = []; const warnings: string[] = []; - // User level: ~/.codex/skills/ const userSkillsDir = join(ctx.home, SOURCE_PATHS.codex.userBase, "skills"); - const userResult = loadFilesFromDir(ctx, userSkillsDir, PROVIDER_ID, "user", { - extensions: ["md"], - recursive: true, - transform: (name, content, path, source) => { - const { frontmatter, body } = parseFrontmatter(content); - const skillName = frontmatter.name || name.replace(/\.md$/, ""); - - return { - name: String(skillName), - path, - content: body, - frontmatter, - level: "user" as const, - _source: source, - }; - }, + const userResult = loadSkillsFromDir(ctx, { + dir: userSkillsDir, + providerId: PROVIDER_ID, + level: "user", }); items.push(...userResult.items); - warnings.push(...(userResult.warnings || [])); + if (userResult.warnings) warnings.push(...userResult.warnings); - // Project level: .codex/skills/ const codexDir = ctx.fs.walkUp(".codex", { dir: true }); if (codexDir) { const projectSkillsDir = join(codexDir, "skills"); - const projectResult = loadFilesFromDir(ctx, projectSkillsDir, PROVIDER_ID, "project", { - extensions: ["md"], - recursive: true, - transform: (name, content, path, source) => { - const { frontmatter, body } = parseFrontmatter(content); - const skillName = frontmatter.name || name.replace(/\.md$/, ""); - - return { - name: String(skillName), - path, - content: body, - frontmatter, - level: "project" as const, - _source: source, - }; - }, + const projectResult = loadSkillsFromDir(ctx, { + dir: projectSkillsDir, + providerId: PROVIDER_ID, + level: "project", }); items.push(...projectResult.items); - warnings.push(...(projectResult.warnings || [])); + if (projectResult.warnings) warnings.push(...projectResult.warnings); } return { items, warnings }; diff --git a/packages/coding-agent/src/discovery/helpers.ts b/packages/coding-agent/src/discovery/helpers.ts index 7a656ef6b..ec6566fca 100644 --- a/packages/coding-agent/src/discovery/helpers.ts +++ b/packages/coding-agent/src/discovery/helpers.ts @@ -4,6 +4,7 @@ import { join, resolve } from "path"; import { parse as parseYAML } from "yaml"; +import type { Skill, SkillFrontmatter } from "../capability/skill"; import type { LoadContext, LoadResult, SourceMeta } from "../capability/types"; /** @@ -126,6 +127,56 @@ export function parseFrontmatter(content: string): { } } +export function loadSkillsFromDir( + ctx: LoadContext, + options: { + dir: string; + providerId: string; + level: "user" | "project"; + requireDescription?: boolean; + }, +): LoadResult { + const items: Skill[] = []; + const warnings: string[] = []; + const { dir, level, providerId, requireDescription = false } = options; + + if (!ctx.fs.isDir(dir)) { + return { items, warnings }; + } + + for (const name of ctx.fs.readDir(dir)) { + if (name.startsWith(".") || name === "node_modules") continue; + + const skillDir = join(dir, name); + if (!ctx.fs.isDir(skillDir)) continue; + + const skillFile = join(skillDir, "SKILL.md"); + if (!ctx.fs.isFile(skillFile)) continue; + + const content = ctx.fs.readFile(skillFile); + if (!content) { + warnings.push(`Failed to read ${skillFile}`); + continue; + } + + const { frontmatter, body } = parseFrontmatter(content); + if (requireDescription && !frontmatter.description) { + continue; + } + + items.push({ + name: (frontmatter.name as string) || name, + path: skillFile, + content: body, + frontmatter: frontmatter as SkillFrontmatter, + level, + _source: createSourceMeta(providerId, skillFile, level), + }); + } + + return { items, warnings }; +} + /** * Expand environment variables in a string. * Supports ${VAR} and ${VAR:-default} syntax. @@ -286,7 +337,7 @@ export function discoverExtensionModulePaths(ctx: LoadContext, dir: string): str const discovered: string[] = []; for (const name of ctx.fs.readDir(dir)) { - if (name.startsWith(".")) continue; + if (name.startsWith(".") || name === "node_modules") continue; const entryPath = join(dir, name); diff --git a/packages/coding-agent/src/prompts/tools/task.md b/packages/coding-agent/src/prompts/tools/task.md index 65f1c6b61..704bc4676 100644 --- a/packages/coding-agent/src/prompts/tools/task.md +++ b/packages/coding-agent/src/prompts/tools/task.md @@ -33,6 +33,7 @@ If you discussed requirements, plans, schemas, or decisions with the user, you M - **Minimize tool chatter**: Avoid repeating large context; use Output tool with output ids for full logs - **Structured completion**: If `output` is provided, subagents must call `complete` to finish - **Parallelize**: Launch multiple agents concurrently whenever possible +- **Isolate file scopes**: Assign each task distinct files or directories so agents don't conflict - **Results are intermediate data**: Agent findings provide context for YOU to perform actual work. Do not treat agent reports as "task complete" signals. - **Stateless invocations**: Subagents have zero memory of your conversation. Pass ALL relevant context: requirements discussed, decisions made, schemas agreed upon, file paths mentioned. If you reference something from earlier discussion without including it, the subagent will fail. - **Trust outputs**: Agent results should generally be trusted