From aa63a5b01db69e4cb8e7e8ed7a858847e42ab5d5 Mon Sep 17 00:00:00 2001 From: MelodyVAR <61931019+MelodyVAR@users.noreply.github.com> Date: Thu, 24 Sep 2026 22:58:59 +0800 Subject: [PATCH] fix: harden skill discovery and preserve loaded instructions --- .../agent-core/src/harness/skill-discovery.ts | 31 +++ packages/agent-core/src/harness/skills.ts | 54 ++--- packages/agent-core/src/index.ts | 1 + packages/coding-agent/docs/skills.md | 12 +- .../coding-agent/src/core/agent-session.ts | 39 +--- .../src/core/compaction/compaction.ts | 34 ++- .../src/core/compaction/skill-instructions.ts | 133 +++++++++++ .../coding-agent/src/core/compaction/utils.ts | 9 +- .../coding-agent/src/core/package-manager.ts | 179 +++++++-------- .../coding-agent/src/core/resource-loader.ts | 4 +- packages/coding-agent/src/core/skills.ts | 62 ++--- .../coding-agent/src/core/system-prompt.ts | 4 +- .../coding-agent/src/utils/skill-block.ts | 22 ++ .../test/resource-loader-no-skills.test.ts | 107 +++++++++ .../test/skill-compaction.test.ts | 183 +++++++++++++++ .../test/skill-loading-regressions.test.ts | 212 ++++++++++++++++++ 16 files changed, 867 insertions(+), 219 deletions(-) create mode 100644 packages/agent-core/src/harness/skill-discovery.ts create mode 100644 packages/coding-agent/src/core/compaction/skill-instructions.ts create mode 100644 packages/coding-agent/src/utils/skill-block.ts create mode 100644 packages/coding-agent/test/resource-loader-no-skills.test.ts create mode 100644 packages/coding-agent/test/skill-compaction.test.ts create mode 100644 packages/coding-agent/test/skill-loading-regressions.test.ts diff --git a/packages/agent-core/src/harness/skill-discovery.ts b/packages/agent-core/src/harness/skill-discovery.ts new file mode 100644 index 00000000..0be7f20b --- /dev/null +++ b/packages/agent-core/src/harness/skill-discovery.ts @@ -0,0 +1,31 @@ +import ignore from "ignore"; + +/** + * Ignore rules scoped to one directory in a resource tree. Paths are relative + * to the scan root, using forward slashes. Callers prune ignored directories + * before reading child rules, as gitignore requires. + */ +export class SkillIgnoreMatcher { + private readonly matcher = ignore(); + private readonly prefix: string; + private readonly parent?: SkillIgnoreMatcher; + + constructor(directory = "", parent?: SkillIgnoreMatcher) { + this.prefix = directory ? `${directory}/` : ""; + this.parent = parent; + } + + add(patterns: string): void { + this.matcher.add(patterns); + } + + ignores(path: string): boolean { + const inherited = this.parent?.ignores(path) ?? false; + if (!path.startsWith(this.prefix)) return inherited; + const localPath = path.slice(this.prefix.length); + if (!localPath) return inherited; + const result = this.matcher.test(localPath); + if (result.unignored) return false; + return result.ignored || inherited; + } +} diff --git a/packages/agent-core/src/harness/skills.ts b/packages/agent-core/src/harness/skills.ts index 44fd828e..3cb89857 100644 --- a/packages/agent-core/src/harness/skills.ts +++ b/packages/agent-core/src/harness/skills.ts @@ -1,13 +1,11 @@ -import ignore from "ignore"; import { parse } from "yaml"; +import { SkillIgnoreMatcher } from "./skill-discovery.ts"; import { type ExecutionEnv, type FileInfo, type Result, type Skill, toError } from "./types.ts"; const MAX_NAME_LENGTH = 64; const MAX_DESCRIPTION_LENGTH = 1024; const IGNORE_FILE_NAMES = [".gitignore", ".ignore", ".fdignore"]; -type IgnoreMatcher = ReturnType; - export type SkillDiagnosticCode = | "file_info_failed" | "list_failed" @@ -68,7 +66,7 @@ export async function loadSkills( } const rootInfo = rootInfoResult.value; if ((await resolveKind(env, rootInfo, diagnostics)) !== "directory") continue; - const result = await loadSkillsFromDirInternal(env, rootInfo.path, true, ignore(), rootInfo.path); + const result = await loadSkillsFromDirInternal(env, rootInfo.path, true, undefined, rootInfo.path, new Set()); skills.push(...result.skills); diagnostics.push(...result.diagnostics); } @@ -105,8 +103,9 @@ async function loadSkillsFromDirInternal( env: ExecutionEnv, dir: string, includeRootFiles: boolean, - ignoreMatcher: IgnoreMatcher, + parentIgnoreMatcher: SkillIgnoreMatcher | undefined, rootDir: string, + visitedDirs: Set, ): Promise<{ skills: Skill[]; diagnostics: SkillDiagnostic[] }> { const skills: Skill[] = []; const diagnostics: SkillDiagnostic[] = []; @@ -126,7 +125,13 @@ async function loadSkillsFromDirInternal( const dirInfo = dirInfoResult.value; if ((await resolveKind(env, dirInfo, diagnostics)) !== "directory") return { skills, diagnostics }; - await addIgnoreRules(env, ignoreMatcher, dir, rootDir, diagnostics); + const canonicalDir = await env.canonicalPath(dir); + const realDir = canonicalDir.ok ? canonicalDir.value : dirInfo.path; + if (visitedDirs.has(realDir)) return { skills, diagnostics }; + visitedDirs.add(realDir); + + const ignoreMatcher = new SkillIgnoreMatcher(relativeEnvPath(rootDir, dir), parentIgnoreMatcher); + await addIgnoreRules(env, ignoreMatcher, dir, diagnostics); const entriesResult = await env.listDir(dir); if (!entriesResult.ok) { @@ -160,7 +165,7 @@ async function loadSkillsFromDirInternal( if (ignoreMatcher.ignores(ignorePath)) continue; if (kind === "directory") { - const result = await loadSkillsFromDirInternal(env, fullPath, false, ignoreMatcher, rootDir); + const result = await loadSkillsFromDirInternal(env, fullPath, false, ignoreMatcher, rootDir, visitedDirs); skills.push(...result.skills); diagnostics.push(...result.diagnostics); continue; @@ -177,14 +182,10 @@ async function loadSkillsFromDirInternal( async function addIgnoreRules( env: ExecutionEnv, - ig: IgnoreMatcher, + ig: SkillIgnoreMatcher, dir: string, - rootDir: string, diagnostics: SkillDiagnostic[], ): Promise { - const relativeDir = relativeEnvPath(rootDir, dir); - const prefix = relativeDir ? `${relativeDir}/` : ""; - for (const filename of IGNORE_FILE_NAMES) { const ignorePathResult = await env.joinPath([dir, filename]); if (!ignorePathResult.ok) { @@ -215,30 +216,8 @@ async function addIgnoreRules( diagnostics.push({ type: "warning", code: "read_failed", message: content.error.message, path: ignorePath }); continue; } - const patterns = content.value - .split(/\r?\n/) - .map((line) => prefixIgnorePattern(line, prefix)) - .filter((line): line is string => Boolean(line)); - if (patterns.length > 0) ig.add(patterns); - } -} - -function prefixIgnorePattern(line: string, prefix: string): string | null { - const trimmed = line.trim(); - if (!trimmed) return null; - if (trimmed.startsWith("#") && !trimmed.startsWith("\\#")) return null; - - let pattern = line; - let negated = false; - if (pattern.startsWith("!")) { - negated = true; - pattern = pattern.slice(1); - } else if (pattern.startsWith("\\!")) { - pattern = pattern.slice(1); + ig.add(content.value); } - if (pattern.startsWith("/")) pattern = pattern.slice(1); - const prefixed = prefix ? `${prefix}${pattern}` : pattern; - return negated ? `!${prefixed}` : prefixed; } async function loadSkillFromFile( @@ -324,7 +303,10 @@ function parseFrontmatter>( content: string, ): Result<{ frontmatter: T; body: string }, Error> { try { - const normalized = content.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); + const normalized = content + .replace(/^\uFEFF/, "") + .replace(/\r\n/g, "\n") + .replace(/\r/g, "\n"); if (!normalized.startsWith("---")) return { ok: true, value: { frontmatter: {} as T, body: normalized } }; const endIndex = normalized.indexOf("\n---", 3); if (endIndex === -1) return { ok: true, value: { frontmatter: {} as T, body: normalized } }; diff --git a/packages/agent-core/src/index.ts b/packages/agent-core/src/index.ts index b6d935a7..f9d1f194 100644 --- a/packages/agent-core/src/index.ts +++ b/packages/agent-core/src/index.ts @@ -93,6 +93,7 @@ export * from "./harness/prompt-templates.ts"; // Harness export * from "./harness/result.ts"; export * from "./harness/session/index.ts"; +export { SkillIgnoreMatcher } from "./harness/skill-discovery.ts"; export * from "./harness/skills.ts"; export * from "./harness/system-prompt.ts"; export type { diff --git a/packages/coding-agent/docs/skills.md b/packages/coding-agent/docs/skills.md index bed45c72..85ad6652 100644 --- a/packages/coding-agent/docs/skills.md +++ b/packages/coding-agent/docs/skills.md @@ -38,8 +38,10 @@ Discovery rules: - In all skill locations, directories containing `SKILL.md` are discovered recursively - In `~/.agents/skills/` and project `.agents/skills/`, root `.md` files are ignored, but nested `.md` files in grouping folders are discovered when they declare skill frontmatter - Root Markdown files other than `SKILL.md` that do not look like skills are ignored silently +- Directory symlinks are followed once per scan, so cycles do not cause repeated traversal +- `.gitignore`, `.ignore`, and `.fdignore` rules are applied relative to the directory containing each ignore file -Disable discovery with `--no-skills` (explicit `--skill` paths still load). +Disable automatic discovery with `--no-skills`. Skills from default paths, settings, and configured packages are not scanned; explicit `--skill` paths still load. ### Using Skills from Other Harnesses @@ -66,11 +68,13 @@ For project-level Claude Code skills, add to `.stepcode/settings.json`: 1. At startup, step scans skill locations and extracts names and descriptions 2. The system prompt includes available skills in XML format per the [specification](https://agentskills.io/integrate-skills) -3. When a task matches, the agent uses `read` to load the full SKILL.md (models don't always do this; use prompting or `/skill:name` to force it) +3. When a task matches, the agent uses the available file-reading tool (`read_file` in Step) to load the full SKILL.md (models don't always do this; use prompting or `/skill:name` to force it) 4. The agent follows the instructions, using relative paths to reference scripts and assets This is progressive disclosure: only descriptions are always in context, full instructions load on-demand. +Built-in context compaction preserves instructions already loaded through file reads or `/skill:name`, along with their locations and any read ranges. Repeated reads are deduplicated, and a new full read replaces the previously loaded version. Instructions are carried into later compactions without reading inactive skills from disk. Extensions that provide their own compaction result remain responsible for the content they preserve. + ## Skill Commands Skills register as `/skill:name` commands: @@ -80,7 +84,7 @@ Skills register as `/skill:name` commands: /skill:pdf-tools extract # Load skill with arguments ``` -Arguments after the command are appended to the skill content as `User: `. +Any whitespace, including a newline or tab, can separate the skill name from its arguments. Arguments are appended after the skill content. Toggle skill commands via `/settings` in interactive mode or in `settings.json`: @@ -186,7 +190,7 @@ Unknown frontmatter fields are ignored. Declared skills with missing descriptions are not loaded. Malformed `SKILL.md` files and `SKILL.md` files without a description produce warnings and are not loaded. Other Markdown files without valid skill frontmatter are ignored. -Name collisions (same name from different locations) warn and keep the first skill found. +Name collisions (same name from different locations) warn and keep the first skill found. In the default locations, project skills take precedence over user skills, including when using the `loadSkills()` SDK helper. ## Example diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index 945f3200..1252f2ec 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -116,32 +116,7 @@ import { createAllToolDefinitions } from "./tools/index.ts"; import { createToolDefinitionFromAgentTool } from "./tools/tool-definition-wrapper.ts"; import { addUsageToTotals, createUsageTotals } from "./usage-totals.ts"; -// ============================================================================ -// Skill Block Parsing -// ============================================================================ - -/** Parsed skill block from a user message */ -export interface ParsedSkillBlock { - name: string; - location: string; - content: string; - userMessage: string | undefined; -} - -/** - * Parse a skill block from message text. - * Returns null if the text doesn't contain a skill block. - */ -export function parseSkillBlock(text: string): ParsedSkillBlock | null { - const match = text.match(/^\n([\s\S]*?)\n<\/skill>(?:\n\n([\s\S]+))?$/); - if (!match) return null; - return { - name: match[1], - location: match[2], - content: match[3], - userMessage: match[4]?.trim() || undefined, - }; -} +export { type ParsedSkillBlock, parseSkillBlock } from "../utils/skill-block.ts"; /** Session-specific events that extend the core AgentEvent */ export type AgentSessionEvent = @@ -1469,7 +1444,7 @@ export class AgentSession { private _expandSkillCommand(text: string): string { if (!text.startsWith("/skill:")) return text; - const spaceIndex = text.indexOf(" "); + const spaceIndex = text.search(/\s/u); const skillName = spaceIndex === -1 ? text.slice(7) : text.slice(7, spaceIndex); const args = spaceIndex === -1 ? "" : text.slice(spaceIndex + 1).trim(); @@ -2080,7 +2055,10 @@ export class AgentSession { const pathEntries = this.sessionManager.getBranch(); const settings = this.settingsManager.getCompactionSettings(); - const preparation = prepareCompaction(pathEntries, settings); + const preparation = prepareCompaction(pathEntries, settings, { + cwd: this._cwd, + skills: this._resourceLoader.getSkills().skills, + }); if (!preparation) { // Check why we can't compact const lastEntry = pathEntries[pathEntries.length - 1]; @@ -2384,7 +2362,10 @@ export class AgentSession { const pathEntries = this.sessionManager.getBranch(); - const preparation = prepareCompaction(pathEntries, settings); + const preparation = prepareCompaction(pathEntries, settings, { + cwd: this._cwd, + skills: this._resourceLoader.getSkills().skills, + }); if (!preparation) { return false; } diff --git a/packages/coding-agent/src/core/compaction/compaction.ts b/packages/coding-agent/src/core/compaction/compaction.ts index 5f3e40f4..ff3feaef 100644 --- a/packages/coding-agent/src/core/compaction/compaction.ts +++ b/packages/coding-agent/src/core/compaction/compaction.ts @@ -22,6 +22,14 @@ import { type SessionEntry, sessionEntryToContextMessages, } from "../session-manager.ts"; +import { + collectSkillInstructions, + formatSkillInstructions, + readSavedSkillInstructions, + type SkillInstruction, + type SkillInstructionContext, + stripSkillInstructions, +} from "./skill-instructions.ts"; import { computeFileLists, createFileOps, @@ -41,6 +49,8 @@ import { export interface CompactionDetails { readFiles: string[]; modifiedFiles: string[]; + /** Loaded skill instructions carried across successive compactions. */ + activeSkills?: SkillInstruction[]; } /** @@ -777,13 +787,16 @@ export interface CompactionPreparation { previousSummary?: string; /** File operations extracted from messagesToSummarize */ fileOps: FileOperations; - /** Compaction settions from settings.jsonl */ + /** Loaded instructions that must survive model-generated summaries. */ + activeSkills?: SkillInstruction[]; + /** Compaction settings from settings.jsonl. */ settings: CompactionSettings; } export function prepareCompaction( pathEntries: SessionEntry[], settings: CompactionSettings, + skillContext?: SkillInstructionContext, ): CompactionPreparation | undefined { if (pathEntries.length > 0 && pathEntries[pathEntries.length - 1].type === "compaction") { return undefined; @@ -798,10 +811,12 @@ export function prepareCompaction( } let previousSummary: string | undefined; + let previousSkills: SkillInstruction[] = []; let boundaryStart = 0; if (prevCompactionIndex >= 0) { const prevCompaction = pathEntries[prevCompactionIndex] as CompactionEntry; - previousSummary = prevCompaction.summary; + previousSkills = prevCompaction.fromHook ? [] : readSavedSkillInstructions(prevCompaction.details); + previousSummary = stripSkillInstructions(prevCompaction.summary, previousSkills); const firstKeptEntryIndex = pathEntries.findIndex((entry) => entry.id === prevCompaction.firstKeptEntryId); boundaryStart = firstKeptEntryIndex >= 0 ? firstKeptEntryIndex : prevCompactionIndex + 1; } @@ -850,6 +865,12 @@ export function prepareCompaction( } } + const activeSkills = collectSkillInstructions( + convertToLlm([...messagesToSummarize, ...turnPrefixMessages]), + previousSkills, + skillContext, + ); + return { firstKeptEntryId, messagesToSummarize, @@ -858,6 +879,7 @@ export function prepareCompaction( tokensBefore, previousSummary, fileOps, + ...(activeSkills.length > 0 ? { activeSkills } : {}), settings, }; } @@ -906,6 +928,7 @@ export async function compact( tokensBefore, previousSummary, fileOps, + activeSkills = [], settings, } = preparation; @@ -978,6 +1001,7 @@ export async function compact( // Compute file lists and append to summary const { readFiles, modifiedFiles } = computeFileLists(fileOps); summary += formatFileOperations(readFiles, modifiedFiles); + summary += formatSkillInstructions(activeSkills); if (!firstKeptEntryId) { throw new Error("First kept entry has no UUID - session may need migration"); @@ -988,7 +1012,11 @@ export async function compact( firstKeptEntryId, tokensBefore, usage: summaryUsage, - details: { readFiles, modifiedFiles } as CompactionDetails, + details: { + readFiles, + modifiedFiles, + ...(activeSkills.length > 0 ? { activeSkills } : {}), + } as CompactionDetails, }; } diff --git a/packages/coding-agent/src/core/compaction/skill-instructions.ts b/packages/coding-agent/src/core/compaction/skill-instructions.ts new file mode 100644 index 00000000..abc18622 --- /dev/null +++ b/packages/coding-agent/src/core/compaction/skill-instructions.ts @@ -0,0 +1,133 @@ +import { dirname } from "node:path"; +import { contentText, type Message, type ToolResultMessage } from "@step-harness/providers"; +import { resolvePath } from "../../utils/paths.ts"; +import { parseSkillBlock } from "../../utils/skill-block.ts"; + +/** Instructions actually loaded in a conversation, including partial reads. */ +export interface SkillInstruction { + location: string; + content: string; + /** Read range, omitted for full-file reads and explicit invocations. */ + range?: string; +} + +export interface SkillInstructionContext { + cwd?: string; + /** Includes single-file skills whose filename is not SKILL.md. */ + skills?: readonly { filePath: string }[]; +} + +interface SkillRead { + location: string; + toolName: string; + range?: string; +} + +/** Identify read results by their call IDs, not by text that resembles a skill. */ +export function collectSkillReadResults( + messages: readonly Message[], + context: SkillInstructionContext = {}, +): Map { + const knownPaths = new Set(context.skills?.map((skill) => resolvePath(skill.filePath, context.cwd))); + const pending = new Map(); + const reads = new Map(); + for (const message of messages) { + if (message.role === "toolResult") { + const call = pending.get(message.toolCallId); + pending.delete(message.toolCallId); + if (call && call.toolName === message.toolName && !message.isError) reads.set(message, call); + continue; + } + if (message.role !== "assistant" || !Array.isArray(message.content)) continue; + for (const call of message.content) { + if (call.type !== "toolCall") continue; + pending.delete(call.id); + if (call.name !== "read" && call.name !== "read_file") continue; + const path = call.arguments?.path; + if (typeof path !== "string") continue; + const location = resolvePath(path, context.cwd); + if (!/(?:^|[/\\])SKILL\.md$/.test(path) && !knownPaths.has(location)) continue; + const { offset, limit, start_line, end_line } = call.arguments; + const start = offset ?? start_line ?? 1; + const range = + start <= 1 && limit === undefined && end_line === undefined + ? undefined + : JSON.stringify({ offset, limit, start_line, end_line }); + pending.set(call.id, { location, toolName: call.name, ...(range ? { range } : {}) }); + } + } + return reads; +} + +/** Read optional metadata from older or extension-written session files safely. */ +export function readSavedSkillInstructions(details: unknown): SkillInstruction[] { + if ( + !details || + typeof details !== "object" || + !("activeSkills" in details) || + !Array.isArray(details.activeSkills) + ) { + return []; + } + return details.activeSkills.filter( + (value): value is SkillInstruction => + value !== null && + typeof value === "object" && + typeof value.location === "string" && + typeof value.content === "string" && + (value.range === undefined || typeof value.range === "string"), + ); +} + +/** Preserve loaded content without reading any additional files from disk. */ +export function collectSkillInstructions( + messages: readonly Message[], + previous: readonly SkillInstruction[] = [], + context: SkillInstructionContext = {}, +): SkillInstruction[] { + const key = (skill: SkillInstruction) => JSON.stringify([skill.location, skill.range]); + const instructions = new Map(previous.map((skill) => [key(skill), skill])); + const reads = collectSkillReadResults(messages, context); + const remember = (skill: SkillInstruction) => { + // A fresh full read replaces old instructions, including any old ranges. + if (!skill.range) { + for (const [id, existing] of instructions) { + if (existing.location === skill.location) instructions.delete(id); + } + } + instructions.set(key(skill), skill); + }; + + for (const message of messages) { + if (message.role === "user") { + const block = parseSkillBlock(contentText(message.content, "")); + if (block) remember({ location: resolvePath(block.location, context.cwd), content: block.content }); + } else if (message.role === "toolResult" && !message.isError) { + const call = reads.get(message); + if (!call || call.toolName !== message.toolName) continue; + const content = contentText(message.content, ""); + if (!content) continue; + remember({ location: call.location, content, ...(call.range ? { range: call.range } : {}) }); + } + } + return [...instructions.values()]; +} + +function escapeAttribute(value: string): string { + return value.replace(/&/g, "&").replace(/"/g, """).replace(//g, ">"); +} + +export function formatSkillInstructions(skills: readonly SkillInstruction[]): string { + if (skills.length === 0) return ""; + const blocks = skills.map( + (skill) => + `\nReferences are relative to ${dirname(skill.location)}.\n\n${skill.content}\n`, + ); + return `\n\n\nPreviously loaded skill instructions, preserved verbatim:\n\n${blocks.join("\n\n")}\n`; +} + +/** Keep durable instructions outside the next model-generated summary. */ +export function stripSkillInstructions(summary: string, skills: readonly SkillInstruction[]): string { + const suffix = formatSkillInstructions(skills); + return suffix && summary.endsWith(suffix) ? summary.slice(0, -suffix.length) : summary; +} diff --git a/packages/coding-agent/src/core/compaction/utils.ts b/packages/coding-agent/src/core/compaction/utils.ts index bd232b82..b39ddbe9 100644 --- a/packages/coding-agent/src/core/compaction/utils.ts +++ b/packages/coding-agent/src/core/compaction/utils.ts @@ -4,6 +4,7 @@ import type { AgentMessage } from "@step-harness/agent-core"; import { contentText, type Message } from "@step-harness/providers"; +import { collectSkillReadResults } from "./skill-instructions.ts"; /** File paths touched by a session branch or compaction range. */ export interface FileOperations { @@ -42,12 +43,15 @@ export function extractFileOpsFromMessage(message: AgentMessage, fileOps: FileOp switch (block.name) { case "read": + case "read_file": fileOps.read.add(path); break; case "write": + case "write_file": fileOps.written.add(path); break; case "edit": + case "edit_file": fileOps.edited.add(path); break; } @@ -168,6 +172,7 @@ export function serializeConversation( toolResultTruncation: ToolResultTruncationOptions = DEFAULT_TOOL_RESULT_TRUNCATION, ): string { const parts: string[] = []; + const skillReads = collectSkillReadResults(messages); for (const msg of messages) { if (msg.role === "user") { @@ -201,7 +206,9 @@ export function serializeConversation( } else if (msg.role === "toolResult") { const content = contentText(msg.content, ""); if (content) { - parts.push(`[Tool result]: ${truncateForSummary(content, toolResultTruncation)}`); + const skillRead = skillReads.get(msg); + const preserve = !msg.isError && skillRead?.toolName === msg.toolName; + parts.push(`[Tool result]: ${preserve ? content : truncateForSummary(content, toolResultTruncation)}`); } } } diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 9f3e7654..90684de8 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -34,7 +34,7 @@ function getEnv(): NodeJS.ProcessEnv { import { basename, dirname, join, relative, resolve, sep } from "node:path"; import type { Readable } from "node:stream"; -import ignore from "ignore"; +import { SkillIgnoreMatcher } from "@step-harness/agent-core"; import { minimatch } from "minimatch"; import { gt, maxSatisfying, rcompare, satisfies, valid, validRange } from "semver"; import { CONFIG_DIR_NAME } from "../config.ts"; @@ -104,7 +104,10 @@ export interface ConfiguredPackage { } export interface PackageManager { - resolve(onMissing?: (source: string) => Promise): Promise; + resolve( + onMissing?: (source: string) => Promise, + options?: { includeSkills?: boolean }, + ): Promise; install(source: string, options?: { local?: boolean }): Promise; installAndPersist(source: string, options?: { local?: boolean }): Promise; remove(source: string, options?: { local?: boolean }): Promise; @@ -163,6 +166,7 @@ interface GitUpdateTarget extends ConfiguredUpdateSource { } interface ResourceAccumulator { + resourceTypes: readonly ResourceType[]; extensions: Map; skills: Map; prompts: Map; @@ -208,8 +212,6 @@ const FILE_PATTERNS: Record = { const IGNORE_FILE_NAMES = [".gitignore", ".ignore", ".fdignore"]; -type IgnoreMatcher = ReturnType; - function toPosixPath(p: string): string { return p.split(sep).join("/"); } @@ -225,45 +227,10 @@ export function getExtensionTempFolder(agentDir: string): string { return tempFolder; } -function prefixIgnorePattern(line: string, prefix: string): string | null { - const trimmed = line.trim(); - if (!trimmed) return null; - if (trimmed.startsWith("#") && !trimmed.startsWith("\\#")) return null; - - let pattern = line; - let negated = false; - - if (pattern.startsWith("!")) { - negated = true; - pattern = pattern.slice(1); - } else if (pattern.startsWith("\\!")) { - pattern = pattern.slice(1); - } - - if (pattern.startsWith("/")) { - pattern = pattern.slice(1); - } - - const prefixed = prefix ? `${prefix}${pattern}` : pattern; - return negated ? `!${prefixed}` : prefixed; -} - -function addIgnoreRules(ig: IgnoreMatcher, dir: string, rootDir: string): void { - const relativeDir = relative(rootDir, dir); - const prefix = relativeDir ? `${toPosixPath(relativeDir)}/` : ""; - +function addIgnoreRules(ig: SkillIgnoreMatcher, dir: string): void { for (const filename of IGNORE_FILE_NAMES) { - const ignorePath = join(dir, filename); - if (!existsSync(ignorePath)) continue; try { - const content = readFileSync(ignorePath, "utf-8"); - const patterns = content - .split(/\r?\n/) - .map((line) => prefixIgnorePattern(line, prefix)) - .filter((line): line is string => Boolean(line)); - if (patterns.length > 0) { - ig.add(patterns); - } + ig.add(readFileSync(join(dir, filename), "utf-8")); } catch {} } } @@ -309,15 +276,20 @@ function collectFiles( dir: string, filePattern: RegExp, skipNodeModules = true, - ignoreMatcher?: IgnoreMatcher, + ignoreMatcher?: SkillIgnoreMatcher, rootDir?: string, + visitedDirs = new Set(), ): string[] { const files: string[] = []; if (!existsSync(dir)) return files; + const realDir = canonicalizePath(dir); + if (visitedDirs.has(realDir)) return []; + visitedDirs.add(realDir); + const root = rootDir ?? dir; - const ig = ignoreMatcher ?? ignore(); - addIgnoreRules(ig, dir, root); + const ig = new SkillIgnoreMatcher(toPosixPath(relative(root, dir)), ignoreMatcher); + addIgnoreRules(ig, dir); try { const entries = readdirSync(dir, { withFileTypes: true }); @@ -344,7 +316,7 @@ function collectFiles( if (ig.ignores(ignorePath)) continue; if (isDir) { - files.push(...collectFiles(fullPath, filePattern, skipNodeModules, ig, root)); + files.push(...collectFiles(fullPath, filePattern, skipNodeModules, ig, root, visitedDirs)); } else if (isFile && filePattern.test(entry.name)) { files.push(fullPath); } @@ -361,15 +333,20 @@ type SkillDiscoveryMode = "pi" | "agents"; function collectSkillEntries( dir: string, mode: SkillDiscoveryMode, - ignoreMatcher?: IgnoreMatcher, + ignoreMatcher?: SkillIgnoreMatcher, rootDir?: string, + visitedDirs = new Set(), ): string[] { const entries: string[] = []; if (!existsSync(dir)) return entries; + const realDir = canonicalizePath(dir); + if (visitedDirs.has(realDir)) return []; + visitedDirs.add(realDir); + const root = rootDir ?? dir; - const ig = ignoreMatcher ?? ignore(); - addIgnoreRules(ig, dir, root); + const ig = new SkillIgnoreMatcher(toPosixPath(relative(root, dir)), ignoreMatcher); + addIgnoreRules(ig, dir); try { const dirEntries = readdirSync(dir, { withFileTypes: true }); @@ -428,7 +405,7 @@ function collectSkillEntries( if (!isDir) continue; if (ig.ignores(`${relPath}/`)) continue; - entries.push(...collectSkillEntries(fullPath, mode, ig, root)); + entries.push(...collectSkillEntries(fullPath, mode, ig, root, visitedDirs)); } } catch { // Ignore errors @@ -480,8 +457,8 @@ function collectAutoPromptEntries(dir: string): string[] { const entries: string[] = []; if (!existsSync(dir)) return entries; - const ig = ignore(); - addIgnoreRules(ig, dir, dir); + const ig = new SkillIgnoreMatcher(); + addIgnoreRules(ig, dir); try { const dirEntries = readdirSync(dir, { withFileTypes: true }); @@ -517,8 +494,8 @@ function collectAutoThemeEntries(dir: string): string[] { const entries: string[] = []; if (!existsSync(dir)) return entries; - const ig = ignore(); - addIgnoreRules(ig, dir, dir); + const ig = new SkillIgnoreMatcher(); + addIgnoreRules(ig, dir); try { const dirEntries = readdirSync(dir, { withFileTypes: true }); @@ -591,8 +568,8 @@ function collectAutoExtensionEntries(dir: string): string[] { } // Otherwise, discover extensions from directory contents - const ig = ignore(); - addIgnoreRules(ig, dir, dir); + const ig = new SkillIgnoreMatcher(); + addIgnoreRules(ig, dir); try { const dirEntries = readdirSync(dir, { withFileTypes: true }); @@ -907,8 +884,11 @@ export class DefaultPackageManager implements PackageManager { } } - async resolve(onMissing?: (source: string) => Promise): Promise { - const accumulator = this.createAccumulator(); + async resolve( + onMissing?: (source: string) => Promise, + options?: { includeSkills?: boolean }, + ): Promise { + const accumulator = this.createAccumulator(options?.includeSkills); const globalSettings = this.settingsManager.getGlobalSettings(); const projectSettings = this.settingsManager.getProjectSettings(); @@ -928,7 +908,7 @@ export class DefaultPackageManager implements PackageManager { const globalBaseDir = this.agentDir; const projectBaseDir = join(this.cwd, this.configDirName); - for (const resourceType of RESOURCE_TYPES) { + for (const resourceType of accumulator.resourceTypes) { const target = this.getTargetMap(accumulator, resourceType); const globalEntries = (globalSettings[resourceType] ?? []) as string[]; const projectEntries = (projectSettings[resourceType] ?? []) as string[]; @@ -2139,7 +2119,7 @@ export class DefaultPackageManager implements PackageManager { metadata: PathMetadata, ): boolean { if (filter) { - for (const resourceType of RESOURCE_TYPES) { + for (const resourceType of accumulator.resourceTypes) { const patterns = filter[resourceType]; const target = this.getTargetMap(accumulator, resourceType); if (filter.autoload === false) { @@ -2155,7 +2135,7 @@ export class DefaultPackageManager implements PackageManager { const manifest = readPiManifest(join(packageRoot, "package.json")); if (manifest) { - for (const resourceType of RESOURCE_TYPES) { + for (const resourceType of accumulator.resourceTypes) { const entries = manifest[resourceType as keyof PiManifest]; this.addManifestEntries( entries, @@ -2172,12 +2152,14 @@ export class DefaultPackageManager implements PackageManager { for (const resourceType of RESOURCE_TYPES) { const dir = join(packageRoot, resourceType); if (existsSync(dir)) { - // Collect all files from the directory (all enabled by default) + // Disabled resource directories still identify a resource package; + // do not reinterpret a skills-only package as an extension. + hasAnyDir = true; + if (!accumulator.resourceTypes.includes(resourceType)) continue; const files = collectResourceFiles(dir, resourceType); for (const f of files) { this.addResource(this.getTargetMap(accumulator, resourceType), f, metadata, true); } - hasAnyDir = true; } } return hasAnyDir; @@ -2378,9 +2360,11 @@ export class DefaultPackageManager implements PackageManager { }; const userAgentsSkillsDir = join(getHomeDir(), ".agents", "skills"); const projectTrusted = this.settingsManager.isProjectTrusted(); - const projectAgentsSkillDirs = projectTrusted - ? collectAncestorAgentsSkillDirs(this.cwd).filter((dir) => resolve(dir) !== resolve(userAgentsSkillsDir)) - : []; + const includeSkills = accumulator.resourceTypes.includes("skills"); + const projectAgentsSkillDirs = + includeSkills && projectTrusted + ? collectAncestorAgentsSkillDirs(this.cwd).filter((dir) => resolve(dir) !== resolve(userAgentsSkillsDir)) + : []; const addResources = ( resourceType: ResourceType, @@ -2406,14 +2390,16 @@ export class DefaultPackageManager implements PackageManager { projectBaseDir, ); - // Project skills from .pi/ - addResources( - "skills", - collectAutoSkillEntries(projectDirs.skills, "pi"), - projectMetadata, - projectOverrides.skills, - projectBaseDir, - ); + // Project skills from the product's configuration directory. + if (includeSkills) { + addResources( + "skills", + collectAutoSkillEntries(projectDirs.skills, "pi"), + projectMetadata, + projectOverrides.skills, + projectBaseDir, + ); + } } // Project skills from .agents/ (each with its own baseDir) @@ -2458,28 +2444,30 @@ export class DefaultPackageManager implements PackageManager { globalBaseDir, ); - // User skills from ~/.pi/agent/ - addResources( - "skills", - collectAutoSkillEntries(userDirs.skills, "pi"), - userMetadata, - userOverrides.skills, - globalBaseDir, - ); + if (includeSkills) { + // User skills from ~/.pi/agent/ + addResources( + "skills", + collectAutoSkillEntries(userDirs.skills, "pi"), + userMetadata, + userOverrides.skills, + globalBaseDir, + ); - // User skills from ~/.agents/ (with its own baseDir) - const userAgentsBaseDir = dirname(userAgentsSkillsDir); - const userAgentsMetadata: PathMetadata = { - ...userMetadata, - baseDir: userAgentsBaseDir, - }; - addResources( - "skills", - collectAutoSkillEntries(userAgentsSkillsDir, "agents"), - userAgentsMetadata, - userOverrides.skills, - userAgentsBaseDir, - ); + // User skills from ~/.agents/ (with its own baseDir) + const userAgentsBaseDir = dirname(userAgentsSkillsDir); + const userAgentsMetadata: PathMetadata = { + ...userMetadata, + baseDir: userAgentsBaseDir, + }; + addResources( + "skills", + collectAutoSkillEntries(userAgentsSkillsDir, "agents"), + userAgentsMetadata, + userOverrides.skills, + userAgentsBaseDir, + ); + } addResources( "prompts", @@ -2546,8 +2534,9 @@ export class DefaultPackageManager implements PackageManager { } } - private createAccumulator(): ResourceAccumulator { + private createAccumulator(includeSkills = true): ResourceAccumulator { return { + resourceTypes: includeSkills ? RESOURCE_TYPES : RESOURCE_TYPES.filter((type) => type !== "skills"), extensions: new Map(), skills: new Map(), prompts: new Map(), diff --git a/packages/coding-agent/src/core/resource-loader.ts b/packages/coding-agent/src/core/resource-loader.ts index 9e68f453..81e9adfa 100644 --- a/packages/coding-agent/src/core/resource-loader.ts +++ b/packages/coding-agent/src/core/resource-loader.ts @@ -405,7 +405,7 @@ export class DefaultResourceLoader implements ResourceLoader { // reload() preserves SettingsManager.projectTrusted and reloads settings for that trust state. await this.settingsManager.reload(); - const resolvedPaths = await this.packageManager.resolve(); + const resolvedPaths = await this.packageManager.resolve(undefined, { includeSkills: !this.noSkills }); const cliExtensionPaths = await this.packageManager.resolveExtensionSources(this.additionalExtensionPaths, { temporary: true, }); @@ -551,7 +551,7 @@ export class DefaultResourceLoader implements ResourceLoader { } private async loadCurrentExtensionSet(options: { includeInlineFactories: boolean }): Promise { - const resolvedPaths = await this.packageManager.resolve(); + const resolvedPaths = await this.packageManager.resolve(undefined, { includeSkills: false }); const cliExtensionPaths = await this.packageManager.resolveExtensionSources(this.additionalExtensionPaths, { temporary: true, }); diff --git a/packages/coding-agent/src/core/skills.ts b/packages/coding-agent/src/core/skills.ts index d5d9dade..31da06bc 100644 --- a/packages/coding-agent/src/core/skills.ts +++ b/packages/coding-agent/src/core/skills.ts @@ -1,5 +1,5 @@ +import { SkillIgnoreMatcher } from "@step-harness/agent-core"; import { existsSync, readdirSync, readFileSync, statSync } from "fs"; -import ignore from "ignore"; import { basename, dirname, join, relative, resolve, sep } from "path"; import { CONFIG_DIR_NAME, getAgentDir } from "../config.ts"; import { parseFrontmatter } from "../utils/frontmatter.ts"; @@ -15,51 +15,14 @@ const MAX_DESCRIPTION_LENGTH = 1024; const IGNORE_FILE_NAMES = [".gitignore", ".ignore", ".fdignore"]; -type IgnoreMatcher = ReturnType; - function toPosixPath(p: string): string { return p.split(sep).join("/"); } -function prefixIgnorePattern(line: string, prefix: string): string | null { - const trimmed = line.trim(); - if (!trimmed) return null; - if (trimmed.startsWith("#") && !trimmed.startsWith("\\#")) return null; - - let pattern = line; - let negated = false; - - if (pattern.startsWith("!")) { - negated = true; - pattern = pattern.slice(1); - } else if (pattern.startsWith("\\!")) { - pattern = pattern.slice(1); - } - - if (pattern.startsWith("/")) { - pattern = pattern.slice(1); - } - - const prefixed = prefix ? `${prefix}${pattern}` : pattern; - return negated ? `!${prefixed}` : prefixed; -} - -function addIgnoreRules(ig: IgnoreMatcher, dir: string, rootDir: string): void { - const relativeDir = relative(rootDir, dir); - const prefix = relativeDir ? `${toPosixPath(relativeDir)}/` : ""; - +function addIgnoreRules(ig: SkillIgnoreMatcher, dir: string): void { for (const filename of IGNORE_FILE_NAMES) { - const ignorePath = join(dir, filename); - if (!existsSync(ignorePath)) continue; try { - const content = readFileSync(ignorePath, "utf-8"); - const patterns = content - .split(/\r?\n/) - .map((line) => prefixIgnorePattern(line, prefix)) - .filter((line): line is string => Boolean(line)); - if (patterns.length > 0) { - ig.add(patterns); - } + ig.add(readFileSync(join(dir, filename), "utf-8")); } catch {} } } @@ -174,8 +137,9 @@ function loadSkillsFromDirInternal( dir: string, source: string, includeRootFiles: boolean, - ignoreMatcher?: IgnoreMatcher, + ignoreMatcher?: SkillIgnoreMatcher, rootDir?: string, + visitedDirs = new Set(), ): LoadSkillsResult { const skills: Skill[] = []; const diagnostics: ResourceDiagnostic[] = []; @@ -184,9 +148,13 @@ function loadSkillsFromDirInternal( return { skills, diagnostics }; } + const realDir = canonicalizePath(dir); + if (visitedDirs.has(realDir)) return { skills, diagnostics }; + visitedDirs.add(realDir); + const root = rootDir ?? dir; - const ig = ignoreMatcher ?? ignore(); - addIgnoreRules(ig, dir, root); + const ig = new SkillIgnoreMatcher(toPosixPath(relative(root, dir)), ignoreMatcher); + addIgnoreRules(ig, dir); try { const entries = readdirSync(dir, { withFileTypes: true }); @@ -253,7 +221,7 @@ function loadSkillsFromDirInternal( } if (isDirectory) { - const subResult = loadSkillsFromDirInternal(fullPath, source, false, ig, root); + const subResult = loadSkillsFromDirInternal(fullPath, source, false, ig, root, visitedDirs); skills.push(...subResult.skills); diagnostics.push(...subResult.diagnostics); continue; @@ -352,7 +320,7 @@ function loadSkillFromFile( * Skills with disableModelInvocation=true are excluded from the prompt * (they can only be invoked explicitly via /skill:name commands). */ -export function formatSkillsForPrompt(skills: Skill[]): string { +export function formatSkillsForPrompt(skills: Skill[], readTool: "read" | "read_file" = "read"): string { const visibleSkills = skills.filter((s) => !s.disableModelInvocation); if (visibleSkills.length === 0) { @@ -361,7 +329,7 @@ export function formatSkillsForPrompt(skills: Skill[]): string { const lines = [ "\n\nThe following skills provide specialized instructions for specific tasks.", - "Use the read tool to load a skill's file when the task matches its description.", + `Use the ${readTool} tool to load a skill's file when the task matches its description.`, "When a skill file references a relative path, resolve it against the skill directory (parent of SKILL.md / dirname of the path) and use that absolute path in tool commands.", "", "", @@ -451,8 +419,8 @@ export function loadSkills(options: LoadSkillsOptions): LoadSkillsResult { } if (includeDefaults) { - addSkills(loadSkillsFromDirInternal(join(resolvedAgentDir, "skills"), "user", true)); addSkills(loadSkillsFromDirInternal(resolve(resolvedCwd, configDirName, "skills"), "project", true)); + addSkills(loadSkillsFromDirInternal(join(resolvedAgentDir, "skills"), "user", true)); } const userSkillsDir = join(resolvedAgentDir, "skills"); diff --git a/packages/coding-agent/src/core/system-prompt.ts b/packages/coding-agent/src/core/system-prompt.ts index e131c59d..702f5f5a 100644 --- a/packages/coding-agent/src/core/system-prompt.ts +++ b/packages/coding-agent/src/core/system-prompt.ts @@ -114,7 +114,7 @@ export function buildSystemPrompt(options: BuildSystemPromptOptions): string { const customPromptHasRead = !selectedTools || selectedTools.some((name) => name === "read" || name === "read_file"); if (customPromptHasRead && skills.length > 0) { - prompt += formatSkillsForPrompt(skills); + prompt += formatSkillsForPrompt(skills, activeToolNames.includes("read") ? "read" : "read_file"); } if (productAppendix) { prompt += productAppendix; @@ -218,7 +218,7 @@ ${ // Append skills section (only if read tool is available) if (hasRead && skills.length > 0) { - prompt += formatSkillsForPrompt(skills); + prompt += formatSkillsForPrompt(skills, activeToolNames.includes("read") ? "read" : "read_file"); } if (productAppendix) { prompt += productAppendix; diff --git a/packages/coding-agent/src/utils/skill-block.ts b/packages/coding-agent/src/utils/skill-block.ts new file mode 100644 index 00000000..d173d2bf --- /dev/null +++ b/packages/coding-agent/src/utils/skill-block.ts @@ -0,0 +1,22 @@ +/** Parsed skill block from a user message */ +export interface ParsedSkillBlock { + name: string; + location: string; + content: string; + userMessage: string | undefined; +} + +/** + * Parse a skill block from message text. + * Returns null if the text doesn't contain a skill block. + */ +export function parseSkillBlock(text: string): ParsedSkillBlock | null { + const match = text.match(/^\n([\s\S]*?)\n<\/skill>(?:\n\n([\s\S]+))?$/); + if (!match) return null; + return { + name: match[1], + location: match[2], + content: match[3], + userMessage: match[4]?.trim() || undefined, + }; +} diff --git a/packages/coding-agent/test/resource-loader-no-skills.test.ts b/packages/coding-agent/test/resource-loader-no-skills.test.ts new file mode 100644 index 00000000..d5f440a8 --- /dev/null +++ b/packages/coding-agent/test/resource-loader-no-skills.test.ts @@ -0,0 +1,107 @@ +import { mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, sep } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { DefaultResourceLoader } from "../src/core/resource-loader.ts"; +import { SettingsManager } from "../src/core/settings-manager.ts"; + +vi.mock("node:fs", async (importOriginal) => { + const fs = await importOriginal(); + return { ...fs, readdirSync: vi.fn(fs.readdirSync) }; +}); + +const roots: string[] = []; +afterEach(() => { + vi.clearAllMocks(); + for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); +}); + +function writeSkill(dir: string, name: string): void { + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "SKILL.md"), `---\nname: ${name}\ndescription: ${name} instructions\n---\nUse ${name}.`); +} + +describe("--no-skills discovery", () => { + it("does not traverse default, configured or package skill directories", async () => { + const root = mkdtempSync(join(tmpdir(), "step-no-skills-")); + roots.push(root); + const cwd = join(root, "project"); + const agentDir = join(root, "agent"); + mkdirSync(cwd, { recursive: true }); + const defaultDir = join(agentDir, "skills"); + const configuredDir = join(root, "configured"); + const packageDir = join(root, "package"); + const packageSkills = join(packageDir, "skills"); + for (const [dir, name] of [ + [defaultDir, "default-skill"], + [configuredDir, "configured-skill"], + [packageSkills, "package-skill"], + ]) { + writeSkill(join(dir, name), name); + } + writeFileSync( + join(packageDir, "package.json"), + JSON.stringify({ name: "skills-package", version: "1.0.0", pi: { skills: ["skills"] } }), + ); + const loader = new DefaultResourceLoader({ + cwd, + agentDir, + noSkills: true, + noExtensions: true, + noContextFiles: true, + settingsManager: SettingsManager.inMemory({ skills: [configuredDir], packages: [packageDir] }), + }); + await loader.reload(); + expect(loader.getSkills().skills).toEqual([]); + const visited = vi.mocked(readdirSync).mock.calls.map(([path]) => String(path)); + expect( + visited.some((path) => + [defaultDir, configuredDir, packageSkills].some((dir) => path === dir || path.startsWith(dir + sep)), + ), + ).toBe(false); + }); + + it("still discovers explicitly supplied skill directories", async () => { + const root = mkdtempSync(join(tmpdir(), "step-explicit-skills-")); + roots.push(root); + const cwd = join(root, "project"); + const agentDir = join(root, "agent"); + const explicitDir = join(root, "explicit"); + mkdirSync(cwd, { recursive: true }); + mkdirSync(agentDir, { recursive: true }); + writeSkill(join(explicitDir, "explicit-skill"), "explicit-skill"); + const loader = new DefaultResourceLoader({ + cwd, + agentDir, + noSkills: true, + noExtensions: true, + noContextFiles: true, + settingsManager: SettingsManager.inMemory(), + additionalSkillPaths: [explicitDir], + }); + await loader.reload(); + expect(loader.getSkills().skills.map((skill) => skill.name)).toEqual(["explicit-skill"]); + }); + + it("does not reinterpret a skills-only convention package as an extension", async () => { + const root = mkdtempSync(join(tmpdir(), "step-skills-package-")); + roots.push(root); + const cwd = join(root, "project"); + const agentDir = join(root, "agent"); + const packageDir = join(root, "package"); + mkdirSync(cwd, { recursive: true }); + mkdirSync(agentDir, { recursive: true }); + writeSkill(join(packageDir, "skills", "package-skill"), "package-skill"); + const loader = new DefaultResourceLoader({ + cwd, + agentDir, + noSkills: true, + noContextFiles: true, + settingsManager: SettingsManager.inMemory({ packages: [packageDir] }), + }); + await loader.reload(); + expect(loader.getExtensions().errors).toEqual([]); + expect(loader.getExtensions().extensions).toEqual([]); + expect(loader.getSkills().skills).toEqual([]); + }); +}); diff --git a/packages/coding-agent/test/skill-compaction.test.ts b/packages/coding-agent/test/skill-compaction.test.ts new file mode 100644 index 00000000..c39ffdc2 --- /dev/null +++ b/packages/coding-agent/test/skill-compaction.test.ts @@ -0,0 +1,183 @@ +import { dirname } from "node:path"; +import { fauxAssistantMessage, fauxToolCall, type Message } from "@step-harness/providers"; +import { afterEach, describe, expect, it } from "vitest"; +import { buildSessionContext } from "../src/core/session-manager.ts"; +import { createSyntheticSourceInfo } from "../src/core/source-info.ts"; +import { createHarness, type Harness } from "./suite/harness.ts"; +import { createTestResourceLoader } from "./utilities.ts"; + +const harnesses: Harness[] = []; +afterEach(() => { + for (const harness of harnesses.splice(0)) harness.cleanup(); +}); + +async function makeHarness(paths: string[] = []) { + const harness = await createHarness({ + settings: { compaction: { keepRecentTokens: 1 } }, + resourceLoader: { + ...createTestResourceLoader(), + getSkills: () => ({ + skills: paths.map((filePath, index) => ({ + name: `skill-${index}`, + description: "Skill instructions", + filePath, + baseDir: dirname(filePath), + sourceInfo: createSyntheticSourceInfo(filePath, { source: "local" }), + disableModelInvocation: false, + })), + diagnostics: [], + }), + }, + }); + harnesses.push(harness); + return harness; +} + +function readSkill( + path: string, + body: string, + range: Record = {}, + isError = false, + callId?: string, +): Message[] { + const call = fauxToolCall("read_file", { path, ...range }); + if (callId) call.id = callId; + return [ + fauxAssistantMessage(call), + { + role: "toolResult", + toolCallId: call.id, + toolName: "read_file", + content: [{ type: "text", text: body }], + isError, + timestamp: Date.now(), + }, + ]; +} + +function seed(harness: Harness, messages: Message[]): void { + for (const message of [ + { role: "user" as const, content: "Use the relevant skills", timestamp: Date.now() }, + ...messages, + fauxAssistantMessage("The instructions are loaded."), + { role: "user" as const, content: "Continue with the current task", timestamp: Date.now() }, + fauxAssistantMessage("Current progress."), + ]) + harness.sessionManager.appendMessage(message); + harness.session.agent.state.messages = harness.sessionManager.buildSessionContext().messages; +} + +function summarizeWithoutSkills(harness: Harness): void { + harness.setResponses([ + fauxAssistantMessage("Brief task handoff that omits the loaded instructions."), + fauxAssistantMessage("Current turn handoff that also omits the loaded instructions."), + ]); +} + +describe("skill instructions across compaction", () => { + it("retains complete loaded instructions even when the generated summary omits them", async () => { + const harness = await makeHarness(); + const marker = "Always use the amber-lark deployment name."; + const body = `${"Background.\n".repeat(180)}${marker}\n${"More context.\n".repeat(180)}`; + seed(harness, readSkill("/skills/deploy/SKILL.md", body)); + summarizeWithoutSkills(harness); + + const result = await harness.session.compact(); + expect(result.summary).toContain(body); + expect(result.summary).toContain("/skills/deploy/SKILL.md"); + + const restored = buildSessionContext(JSON.parse(JSON.stringify(harness.sessionManager.getEntries()))); + const summary = restored.messages.find((message) => message.role === "compactionSummary"); + expect(summary?.role === "compactionSummary" && summary.summary).toContain(marker); + }); + + it("retains activated instructions through consecutive compactions without duplicating them", async () => { + const harness = await makeHarness(); + const marker = "Persist this exact skill instruction."; + seed(harness, readSkill("/skills/deploy/SKILL.md", marker)); + summarizeWithoutSkills(harness); + await harness.session.compact(); + + seed(harness, [fauxAssistantMessage("Some later work.")]); + summarizeWithoutSkills(harness); + const result = await harness.session.compact(); + expect(result.summary.split(marker)).toHaveLength(2); + }); + + it("recognizes configured single-file skills and does not load inactive skills", async () => { + const active = "/configured/review.md"; + const inactive = "/configured/unread.md"; + const harness = await makeHarness([active, inactive]); + seed(harness, readSkill(active, "Follow this review procedure.")); + summarizeWithoutSkills(harness); + const result = await harness.session.compact(); + expect(result.summary).toContain("Follow this review procedure."); + expect(result.summary).not.toContain(inactive); + }); + + it("retains slash-invoked instructions without retaining their one-time arguments", async () => { + const harness = await makeHarness(); + seed(harness, [ + { + role: "user", + timestamp: Date.now(), + content: + '\nReferences are relative to /skills/review.\n\nUse the review checklist.\n\n\nONE_TIME_ARGUMENT', + }, + ]); + summarizeWithoutSkills(harness); + const result = await harness.session.compact(); + expect(result.summary).toContain("Use the review checklist."); + expect(result.summary).not.toContain("ONE_TIME_ARGUMENT"); + }); + + it("keeps distinct read ranges and deduplicates repeated reads of a range", async () => { + const harness = await makeHarness(); + const path = "/skills/review/SKILL.md"; + seed(harness, [ + ...readSkill(path, "FIRST_PART_INSTRUCTIONS", { start_line: 1, end_line: 40 }), + ...readSkill(path, "SECOND_PART_INSTRUCTIONS", { start_line: 41, end_line: 80 }), + ...readSkill(path, "SECOND_PART_INSTRUCTIONS", { start_line: 41, end_line: 80 }), + ]); + summarizeWithoutSkills(harness); + const result = await harness.session.compact(); + expect(result.summary.split("FIRST_PART_INSTRUCTIONS")).toHaveLength(2); + expect(result.summary.split("SECOND_PART_INSTRUCTIONS")).toHaveLength(2); + }); + + it("replaces stale instructions when the full skill is read again", async () => { + const harness = await makeHarness(); + const path = "/skills/review/SKILL.md"; + seed(harness, [...readSkill(path, "OUTDATED_INSTRUCTIONS"), ...readSkill(path, "CURRENT_INSTRUCTIONS")]); + summarizeWithoutSkills(harness); + const result = await harness.session.compact(); + expect(result.summary).toContain("CURRENT_INSTRUCTIONS"); + expect(result.summary).not.toContain("OUTDATED_INSTRUCTIONS"); + }); + + it("does not retain failed reads or ordinary file contents as skill instructions", async () => { + const harness = await makeHarness(); + seed(harness, [ + ...readSkill("/skills/review/SKILL.md", "READ_FAILURE_OUTPUT", {}, true), + ...readSkill("/project/README.md", "ORDINARY_FILE_CONTENT"), + ]); + summarizeWithoutSkills(harness); + const result = await harness.session.compact(); + expect(result.summary).not.toContain("READ_FAILURE_OUTPUT"); + expect(result.summary).not.toContain("ORDINARY_FILE_CONTENT"); + }); + + it("correlates reused tool-call IDs with the preceding call", async () => { + const harness = await makeHarness(); + seed(harness, [ + ...readSkill("/skills/first/SKILL.md", "FIRST_SKILL_RULE", {}, false, "reused"), + ...readSkill("/project/README.md", "ORDINARY_README", {}, false, "reused"), + ...readSkill("/skills/second/SKILL.md", "SECOND_SKILL_RULE", {}, false, "reused"), + ]); + summarizeWithoutSkills(harness); + const result = await harness.session.compact(); + expect(result.summary).toContain("FIRST_SKILL_RULE"); + expect(result.summary).toContain("SECOND_SKILL_RULE"); + expect(result.summary).not.toContain("ORDINARY_README"); + }); +}); diff --git a/packages/coding-agent/test/skill-loading-regressions.test.ts b/packages/coding-agent/test/skill-loading-regressions.test.ts new file mode 100644 index 00000000..43c146fe --- /dev/null +++ b/packages/coding-agent/test/skill-loading-regressions.test.ts @@ -0,0 +1,212 @@ +import { execFileSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fauxAssistantMessage, fauxToolCall } from "@step-harness/providers"; +import { afterEach, describe, expect, it } from "vitest"; +import { NodeExecutionEnv } from "../../agent-core/src/harness/env/nodejs.ts"; +import { loadSkills as loadCoreSkills } from "../../agent-core/src/harness/skills.ts"; +import { createFileOps, extractFileOpsFromMessage, serializeConversation } from "../src/core/compaction/utils.ts"; +import { DefaultResourceLoader } from "../src/core/resource-loader.ts"; +import { SettingsManager } from "../src/core/settings-manager.ts"; +import { loadSkills, loadSkillsFromDir, type Skill } from "../src/core/skills.ts"; +import { buildSystemPrompt } from "../src/core/system-prompt.ts"; +import { createStepToolProfile } from "../src/step/tool-profile.ts"; +import { createHarness, getMessageText, type Harness } from "./suite/harness.ts"; +import { createTestResourceLoader } from "./utilities.ts"; + +const tempDirs: string[] = []; +const harnesses: Harness[] = []; +function fixture() { + const root = mkdtempSync(join(tmpdir(), "step-skill-discovery-")); + tempDirs.push(root); + const agentDir = join(root, "agent"); + const cwd = join(root, "project"); + mkdirSync(agentDir, { recursive: true }); + mkdirSync(cwd, { recursive: true }); + return { root, agentDir, cwd }; +} +function putSkill(dir: string, name = "audit-skill", body = "AUDIT_SKILL_BODY", prefix = "") { + mkdirSync(dir, { recursive: true }); + const filePath = join(dir, "SKILL.md"); + writeFileSync(filePath, `${prefix}---\nname: ${name}\ndescription: Audit example\n---\n${body}\n`); + return filePath; +} +function resourceLoader(cwd: string, agentDir: string) { + return new DefaultResourceLoader({ + cwd, + agentDir, + configDirName: ".stepcode", + settingsManager: SettingsManager.inMemory(), + noExtensions: true, + noThemes: true, + noPromptTemplates: true, + noContextFiles: true, + }); +} +afterEach(() => { + for (const h of harnesses.splice(0)) h.cleanup(); + for (const path of tempDirs.splice(0)) rmSync(path, { recursive: true, force: true }); +}); + +describe("skill loading regressions", () => { + it("ordinary skill discovery and project precedence work", async () => { + const { cwd, agentDir } = fixture(); + putSkill(join(agentDir, "skills", "audit-skill")); + const projectPath = putSkill(join(cwd, ".stepcode", "skills", "audit-skill")); + const loader = resourceLoader(cwd, agentDir); + await loader.reload(); + expect(loader.getSkills().skills.find((s) => s.name === "audit-skill")?.filePath).toBe(projectPath); + }); + + it("skill loading instruction must name a tool that the Step profile registers", () => { + const { cwd, agentDir } = fixture(); + putSkill(join(agentDir, "skills", "audit-skill")); + const { skills } = loadSkillsFromDir({ dir: join(agentDir, "skills"), source: "user" }); + const names = createStepToolProfile(cwd, { agentDir }).map((t) => t.name); + const prompt = buildSystemPrompt({ cwd, skills, selectedTools: names }); + const instructedTool = prompt.match(/Use the (\S+) tool to load a skill's file/)?.[1]; + expect(names).toContain("read_file"); + expect(names).toContain(instructedTool); + }); + + it("directory symlink cycles must not load the same skill dozens of times", () => { + const { agentDir } = fixture(); + const skillsDir = join(agentDir, "skills"); + putSkill(join(skillsDir, "z-skill")); + symlinkSync(".", join(skillsDir, "loop")); + const result = loadSkillsFromDir({ dir: skillsDir, source: "user" }); + expect(result.skills.filter((s) => s.name === "audit-skill")).toHaveLength(1); + }); + + it("nested gitignore basename patterns must apply to all descendants in that scope", async () => { + const { root, cwd, agentDir } = fixture(); + execFileSync("git", ["init", "-q", root]); + const group = join(agentDir, "skills", "group"); + const ignoredSkill = putSkill(join(group, "deeper", "audit-skill")); + writeFileSync(join(group, ".gitignore"), "SKILL.md\n"); + // Independent reference for the expected ignore semantics. + expect( + execFileSync("git", ["-C", root, "check-ignore", "--no-index", ignoredSkill], { encoding: "utf8" }).trim(), + ).toBe(ignoredSkill); + const loader = resourceLoader(cwd, agentDir); + await loader.reload(); + expect(loader.getSkills().skills.some((s) => s.filePath === ignoredSkill)).toBe(false); + }); + + it.each([" ", "\n", "\t", "\r\n"])( + "explicit skill invocation accepts whitespace separator %j", + async (separator) => { + const { agentDir } = fixture(); + putSkill(join(agentDir, "skills", "audit-skill")); + const result = loadSkillsFromDir({ dir: join(agentDir, "skills"), source: "user" }); + const harness = await createHarness({ + resourceLoader: { + ...createTestResourceLoader(), + getSkills: () => result, + }, + }); + harnesses.push(harness); + let delivered = ""; + harness.setResponses([ + (ctx) => { + const msg = ctx.messages.find((m) => m.role === "user"); + delivered = getMessageText(msg); + return fauxAssistantMessage("ok"); + }, + ]); + await harness.session.prompt(`/skill:audit-skill${separator}apply this`); + expect(delivered).toContain("AUDIT_SKILL_BODY"); + expect(delivered).toContain("apply this"); + }, + ); + + it("public loadSkills and DefaultResourceLoader should agree on the collision winner", async () => { + const { cwd, agentDir } = fixture(); + putSkill(join(agentDir, "skills", "audit-skill"), "audit-skill", "USER"); + putSkill(join(cwd, ".stepcode", "skills", "audit-skill"), "audit-skill", "PROJECT"); + const loader = resourceLoader(cwd, agentDir); + await loader.reload(); + const apiResult = loadSkills({ + cwd, + agentDir, + configDirName: ".stepcode", + skillPaths: [], + includeDefaults: true, + }); + const winner = (skills: Skill[]) => skills.find((s) => s.name === "audit-skill")?.filePath; + expect(winner(apiResult.skills)).toBe(winner(loader.getSkills().skills)); + }); + + it("both loaders should accept the same UTF-8 BOM skill file", async () => { + const { root, agentDir } = fixture(); + const skillsDir = join(agentDir, "skills"); + putSkill(join(skillsDir, "audit-skill"), "audit-skill", "AUDIT_SKILL_BODY", "\uFEFF"); + const cliResult = loadSkillsFromDir({ dir: skillsDir, source: "user" }); + expect(cliResult.skills).toHaveLength(1); + const coreResult = await loadCoreSkills(new NodeExecutionEnv({ cwd: root }), skillsDir); + expect(coreResult.skills).toHaveLength(1); + }); + + it("compaction file tracking should retain the path of a skill loaded by read_file", () => { + const filePath = "/skills/audit-skill/SKILL.md"; + const fileOps = createFileOps(); + extractFileOpsFromMessage(fauxAssistantMessage(fauxToolCall("read_file", { path: filePath })), fileOps); + expect([...fileOps.read]).toContain(filePath); + }); + + it("compaction input should retain the middle instructions of an activated skill", () => { + const marker = "Always name the deployment amber-lark."; + const body = `# Audit skill\n${"Background context.\n".repeat(140)}\n${marker}\n${"Additional context.\n".repeat(140)}`; + const call = fauxToolCall("read_file", { path: "/skills/audit-skill/SKILL.md" }); + const serialized = serializeConversation([ + fauxAssistantMessage(call), + { + role: "toolResult", + toolCallId: call.id, + toolName: "read_file", + content: [{ type: "text", text: body }], + isError: false, + timestamp: 0, + }, + ]); + expect(serialized).toContain(marker); + }); + + it("the execution-environment loader detects directory symlink cycles", async () => { + const { root, agentDir } = fixture(); + const skillsDir = join(agentDir, "skills"); + putSkill(join(skillsDir, "z-skill")); + symlinkSync(".", join(skillsDir, "loop")); + const result = await loadCoreSkills(new NodeExecutionEnv({ cwd: root }), skillsDir); + expect(result.skills.filter((s) => s.name === "audit-skill")).toHaveLength(1); + }); + + it("the execution-environment loader honors nested ignore scopes", async () => { + const { root, agentDir } = fixture(); + const skillsDir = join(agentDir, "skills"); + const group = join(skillsDir, "group"); + putSkill(join(group, "deeper", "hidden"), "hidden"); + writeFileSync(join(group, ".gitignore"), "SKILL.md\n"); + const result = await loadCoreSkills(new NodeExecutionEnv({ cwd: root }), skillsDir); + expect(result.skills).toHaveLength(0); + }); + + it.each(["coding-agent", "agent-core"])( + "%s keeps nested negations scoped to their directory", + async (loaderName) => { + const { root, agentDir } = fixture(); + const skillsDir = join(agentDir, "skills"); + const group = join(skillsDir, "group[1]"); + putSkill(join(group, "deeper", "visible"), "visible"); + putSkill(join(skillsDir, "sibling", "hidden"), "hidden"); + writeFileSync(join(skillsDir, ".gitignore"), "SKILL.md\n"); + writeFileSync(join(group, ".gitignore"), "!SKILL.md\n"); + const result = + loaderName === "coding-agent" + ? loadSkillsFromDir({ dir: skillsDir, source: "user" }) + : await loadCoreSkills(new NodeExecutionEnv({ cwd: root }), skillsDir); + expect(result.skills.map((skill) => skill.name)).toEqual(["visible"]); + }, + ); +});