diff --git a/packages/workspace-server/src/services/skills/parse-skill-references.test.ts b/packages/workspace-server/src/services/skills/parse-skill-references.test.ts new file mode 100644 index 0000000000..ff90409bae --- /dev/null +++ b/packages/workspace-server/src/services/skills/parse-skill-references.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from "vitest"; +import { parseSkillReferences } from "./parse-skill-references"; + +const KNOWN = new Set(["rs-review", "dep-skill", "usr", "foo", "My-Helper"]); + +describe("parseSkillReferences", () => { + it.each([ + ["a bare slash reference", "Run /rs-review on the diff.", ["rs-review"]], + ["a reference at line start", "/rs-review\nthen stop", ["rs-review"]], + [ + "backticked and quoted references", + 'Use `/rs-review` or "/dep-skill" here.', + ["rs-review", "dep-skill"], + ], + ["a parenthesized reference", "(see /dep-skill)", ["dep-skill"]], + ["a wiki-style link", "Details in [[dep-skill]].", ["dep-skill"]], + [ + "mixed reference styles, deduplicated", + "Run /rs-review, then [[rs-review]] again and /dep-skill.", + ["rs-review", "dep-skill"], + ], + ["a sentence-final reference", "First run /dep-skill.", ["dep-skill"]], + [ + "an uppercase frontmatter name", + "Use /My-Helper then [[My-Helper]].", + ["My-Helper"], + ], + ["an unknown skill name", "Run /not-a-skill now.", []], + ["a URL path segment", "See https://example.com/foo for docs.", []], + ["a file path segment", "Look in /usr/bin and /foo/bar.", []], + ["a mid-word slash", "either/foo works", []], + ["an empty body", "", []], + ])("handles %s", (_label, content, expected) => { + expect(parseSkillReferences(content, KNOWN)).toEqual(expected); + }); +}); diff --git a/packages/workspace-server/src/services/skills/parse-skill-references.ts b/packages/workspace-server/src/services/skills/parse-skill-references.ts new file mode 100644 index 0000000000..faa838e15c --- /dev/null +++ b/packages/workspace-server/src/services/skills/parse-skill-references.ts @@ -0,0 +1,33 @@ +const SLASH_REFERENCE_REGEX = + /(?<=^|[\s(`"'[])\/([A-Za-z0-9][A-Za-z0-9._-]*)(?![A-Za-z0-9._/-])/gm; +const WIKI_LINK_REGEX = /\[\[([A-Za-z0-9][A-Za-z0-9._-]*)\]\]/g; + +/** + * Finds `/skill-name` and `[[skill-name]]` references in a SKILL.md body. + * Only names in `knownNames` are returned, so paths (`/usr/bin`), URL + * segments, and unrelated slash-words can't match. A slash reference must + * span its whole path segment — `/foo/bar` never matches a skill named `foo`. + */ +export function parseSkillReferences( + content: string, + knownNames: ReadonlySet, +): string[] { + const references = new Set(); + + for (const regex of [SLASH_REFERENCE_REGEX, WIKI_LINK_REGEX]) { + for (const match of content.matchAll(regex)) { + const name = match[1]; + if (knownNames.has(name)) { + references.add(name); + continue; + } + // dots are valid name chars, so a sentence-final "/dep-skill." captures the period + const withoutTrailingDots = name.replace(/\.+$/, ""); + if (withoutTrailingDots !== name && knownNames.has(withoutTrailingDots)) { + references.add(withoutTrailingDots); + } + } + } + + return [...references]; +} diff --git a/packages/workspace-server/src/services/skills/skills.test.ts b/packages/workspace-server/src/services/skills/skills.test.ts index ae2bb42a05..dc06bf1b91 100644 --- a/packages/workspace-server/src/services/skills/skills.test.ts +++ b/packages/workspace-server/src/services/skills/skills.test.ts @@ -625,6 +625,46 @@ describe("resolveSkillBundleDependencies", () => { .join("\n")}\n---\nbody`; } + it("expands prose references (/name and [[name]]) into dependencies", async () => { + const primary = await createSkill( + repoSkillsDir, + "prose-parent", + `---\nname: prose-parent\ndescription: parent\n---\nRun /prose-dep first, then see [[prose-wiki-dep]]. Ignore /usr/bin and /unknown-skill.`, + ); + const slashDep = await createSkill(repoSkillsDir, "prose-dep"); + const wikiDep = await createSkill(repoSkillsDir, "prose-wiki-dep"); + const service = makeService(); + + const resolved = await service.resolveSkillBundleDependencies([ + ref("prose-parent", primary), + ]); + + expect(resolved.map((r) => r.name)).toEqual([ + "prose-parent", + "prose-dep", + "prose-wiki-dep", + ]); + expect(resolved.map((r) => r.path)).toEqual([primary, slashDep, wikiDep]); + }); + + it("prefers a dependency beside the referencing skill over a same-named skill elsewhere", async () => { + const primary = await createSkill( + repoSkillsDir, + "scoped-parent", + withDeps("scoped-parent", ["helper"]), + ); + const repoHelper = await createSkill(repoSkillsDir, "helper"); + await mkdir(userSkillsHome.dir, { recursive: true }); + await createSkill(userSkillsHome.dir, "helper"); + const service = makeService(); + + const resolved = await service.resolveSkillBundleDependencies([ + ref("scoped-parent", primary), + ]); + + expect(resolved.map((r) => r.path)).toEqual([primary, repoHelper]); + }); + it("expands a tagged skill to include its transitive dependencies", async () => { const primary = await createSkill( repoSkillsDir, diff --git a/packages/workspace-server/src/services/skills/skills.ts b/packages/workspace-server/src/services/skills/skills.ts index 9f5db65f0a..d5de57be0c 100644 --- a/packages/workspace-server/src/services/skills/skills.ts +++ b/packages/workspace-server/src/services/skills/skills.ts @@ -16,6 +16,7 @@ import { parseSkillDependencies, parseSkillFrontmatter, } from "./parse-skill-frontmatter"; +import { parseSkillReferences } from "./parse-skill-references"; import type { BundleLocalSkillInput, BundleLocalSkillOutput, @@ -506,12 +507,12 @@ export class SkillsService { } /** - * Expand a set of tagged skill refs to include their transitively-declared - * dependency skills (SKILL.md `dependencies:`), so a skill that needs another - * (e.g. `/rs-self-review` → `rs-adversarial-review`) pulls its dependency into - * the same cloud run instead of the user having to tag every one by hand. - * Only uploadable local skills are returned; a dependency that resolves to a - * built-in (`bundled`) skill is already present in the sandbox and is skipped. + * Expand a set of tagged skill refs to include their transitive dependency + * skills, so a skill that needs another pulls it into the same cloud run. + * A dependency is either declared (SKILL.md frontmatter `dependencies:`) + * or referenced in the SKILL.md body as `/skill-name` or `[[skill-name]]`. + * Only uploadable local skills are returned; a dependency that resolves to + * a built-in (`bundled`) skill is already in the sandbox and is skipped. */ async resolveSkillBundleDependencies( refs: SkillBundleRef[], @@ -519,14 +520,30 @@ export class SkillsService { if (refs.length === 0) return []; const allSkills = await this.listSkills(); - const findUploadableByName = (name: string): SkillBundleRef | null => { - const match = allSkills.find( + // Prefer a dependency beside the referencing skill (same root), then one + // from the same source, so a repo skill referencing /helper gets its + // sibling rather than a same-named skill from another source. + const findUploadableByName = ( + name: string, + referencedFrom: SkillBundleRef, + ): SkillBundleRef | null => { + const candidates = allSkills.filter( (skill) => skill.name === name && isUploadableSkillSource(skill.source), ); + const match = + candidates.find( + (skill) => + path.dirname(skill.path) === path.dirname(referencedFrom.path), + ) ?? + candidates.find((skill) => skill.source === referencedFrom.source) ?? + candidates[0]; return match && isUploadableSkillSource(match.source) ? { name: match.name, source: match.source, path: match.path } : null; }; + const knownSkillNames: ReadonlySet = new Set( + allSkills.map((skill) => skill.name), + ); const seen = new Set(); const resolved: SkillBundleRef[] = []; @@ -558,7 +575,12 @@ export class SkillsService { path.join(skillDir, "SKILL.md"), "utf-8", ); - dependencyNames = parseSkillDependencies(manifest); + dependencyNames = [ + ...new Set([ + ...parseSkillDependencies(manifest), + ...parseSkillReferences(manifest, knownSkillNames), + ]), + ]; } catch { // A ref we can't read (missing/renamed skill) still uploads on its own; // just skip its dependency expansion rather than failing the whole run. @@ -566,7 +588,7 @@ export class SkillsService { } for (const dependencyName of dependencyNames) { - const dependencyRef = findUploadableByName(dependencyName); + const dependencyRef = findUploadableByName(dependencyName, ref); if ( dependencyRef && !seen.has(`${dependencyRef.source}:${dependencyRef.path}`)