From 32935ef3cf4545b9c14ca5920445a010e9150318 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Sat, 9 May 2026 13:37:34 -0500 Subject: [PATCH] wip --- .../cli/cmd/tui/component/dialog-skill.tsx | 42 +++++++---- .../instance/httpapi/groups/instance.ts | 7 +- .../instance/httpapi/handlers/instance.ts | 13 +++- packages/opencode/src/skill/index.ts | 70 ++++++++++++++++-- packages/opencode/test/session/system.test.ts | 1 + packages/opencode/test/skill/skill.test.ts | 71 +++++++++++++++---- packages/sdk/js/src/v2/gen/sdk.gen.ts | 2 + packages/sdk/js/src/v2/gen/types.gen.ts | 27 +++++-- 8 files changed, 190 insertions(+), 43 deletions(-) diff --git a/packages/opencode/src/cli/cmd/tui/component/dialog-skill.tsx b/packages/opencode/src/cli/cmd/tui/component/dialog-skill.tsx index 4bcd3c7bde..80f9f59bff 100644 --- a/packages/opencode/src/cli/cmd/tui/component/dialog-skill.tsx +++ b/packages/opencode/src/cli/cmd/tui/component/dialog-skill.tsx @@ -2,6 +2,7 @@ import { DialogSelect, type DialogSelectOption } from "@tui/ui/dialog-select" import { createResource, createMemo } from "solid-js" import { useDialog } from "@tui/ui/dialog" import { useSDK } from "@tui/context/sdk" +import path from "path" export type DialogSkillProps = { onSelect: (skill: string) => void @@ -13,23 +14,36 @@ export function DialogSkill(props: DialogSkillProps) { dialog.setSize("large") const [skills] = createResource(async () => { - const result = await sdk.client.app.skills() - return result.data ?? [] + const result = await sdk.client.app.skills({ include: "invalid" }) + return Array.isArray(result.data) ? { skills: result.data, invalid: [] } : (result.data ?? { skills: [], invalid: [] }) }) const options = createMemo[]>(() => { - const list = skills() ?? [] - const maxWidth = Math.max(0, ...list.map((s) => s.name.length)) - return list.map((skill) => ({ - title: skill.name.padEnd(maxWidth), - description: skill.description?.replace(/\s+/g, " ").trim(), - value: skill.name, - category: "Skills", - onSelect: () => { - props.onSelect(skill.name) - dialog.clear() - }, - })) + const list = skills() ?? { skills: [], invalid: [] } + const maxWidth = Math.max( + 0, + ...list.skills.map((s) => s.name.length), + ...list.invalid.map((s) => path.basename(path.dirname(s.path)).length), + ) + return [ + ...list.skills.map((skill) => ({ + title: skill.name.padEnd(maxWidth), + description: skill.description?.replace(/\s+/g, " ").trim(), + value: skill.name, + category: "Skills", + onSelect: () => { + props.onSelect(skill.name) + dialog.clear() + }, + })), + ...list.invalid.map((skill) => ({ + title: path.basename(path.dirname(skill.path)).padEnd(maxWidth), + description: `${skill.reason}: ${skill.message}`, + value: skill.path, + category: "Invalid skills", + disabled: true, + })), + ] }) return diff --git a/packages/opencode/src/server/routes/instance/httpapi/groups/instance.ts b/packages/opencode/src/server/routes/instance/httpapi/groups/instance.ts index f2b0504a05..06f829e7ee 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/groups/instance.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/groups/instance.ts @@ -23,6 +23,10 @@ export const VcsDiffQuery = Schema.Struct({ mode: Vcs.Mode, }) +export const SkillQuery = Schema.Struct({ + include: Schema.optional(Schema.Literals(["invalid"])), +}) + export class ApiVcsApplyError extends Schema.ErrorClass("VcsApplyError")( { name: Schema.Literal("VcsApplyError"), @@ -143,7 +147,8 @@ export const InstanceApi = HttpApi.make("instance") }), ), HttpApiEndpoint.get("skill", InstancePaths.skill, { - success: described(Schema.Array(Skill.Info), "List of skills"), + query: SkillQuery, + success: described(Schema.Union([Schema.Array(Skill.Info), Skill.ListWithInvalid]), "List of skills"), }).annotateMerge( OpenApi.annotations({ identifier: "app.skills", diff --git a/packages/opencode/src/server/routes/instance/httpapi/handlers/instance.ts b/packages/opencode/src/server/routes/instance/httpapi/handlers/instance.ts index 50a7fecfa7..7521775c9f 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/handlers/instance.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/handlers/instance.ts @@ -9,7 +9,7 @@ import { Skill } from "@/skill" import { Effect } from "effect" import { HttpApiBuilder } from "effect/unstable/httpapi" import { InstanceHttpApi } from "../api" -import { ApiVcsApplyError } from "../groups/instance" +import { ApiVcsApplyError, SkillQuery } from "../groups/instance" import { markInstanceForDisposal } from "../lifecycle" export const instanceHandlers = HttpApiBuilder.group(InstanceHttpApi, "instance", (handlers) => @@ -77,8 +77,15 @@ export const instanceHandlers = HttpApiBuilder.group(InstanceHttpApi, "instance" return yield* agent.list() }) - const getSkill = Effect.fn("InstanceHttpApi.skill")(function* () { - return yield* skill.all() + const getSkill = Effect.fn("InstanceHttpApi.skill")(function* (ctx: { query: typeof SkillQuery.Type }) { + const skills = yield* skill.all() + if (ctx.query.include === "invalid") { + return { + skills, + invalid: yield* skill.invalid(), + } + } + return skills }) const getLsp = Effect.fn("InstanceHttpApi.lsp")(function* () { diff --git a/packages/opencode/src/skill/index.ts b/packages/opencode/src/skill/index.ts index 696ab887a7..df9ba0a234 100644 --- a/packages/opencode/src/skill/index.ts +++ b/packages/opencode/src/skill/index.ts @@ -33,6 +33,19 @@ export const Info = Schema.Struct({ }).pipe(withStatics((s) => ({ zod: zod(s) }))) export type Info = Schema.Schema.Type +export const Invalid = Schema.Struct({ + path: Schema.String, + reason: Schema.Literals(["parse", "frontmatter", "schema", "duplicate"]), + message: Schema.String, +}).pipe(withStatics((s) => ({ zod: zod(s) }))) +export type Invalid = Schema.Schema.Type + +export const ListWithInvalid = Schema.Struct({ + skills: Schema.Array(Info), + invalid: Schema.Array(Invalid), +}).pipe(withStatics((s) => ({ zod: zod(s) }))) +export type ListWithInvalid = Schema.Schema.Type + export const InvalidError = NamedError.create( "SkillInvalidError", z.object({ @@ -53,6 +66,7 @@ export const NameMismatchError = NamedError.create( type State = { skills: Record + invalid: Invalid[] dirs: Set } @@ -69,6 +83,7 @@ type ScanState = { export interface Interface { readonly get: (name: string) => Effect.Effect readonly all: () => Effect.Effect + readonly invalid: () => Effect.Effect readonly dirs: () => Effect.Effect readonly available: (agent?: Agent.Info) => Effect.Effect } @@ -80,9 +95,15 @@ const add = Effect.fnUntraced(function* (state: State, match: string, bus: Bus.I }).pipe( Effect.catch( Effect.fnUntraced(function* (err) { - const message = ConfigMarkdown.FrontmatterError.isInstance(err) + const isFrontmatter = ConfigMarkdown.FrontmatterError.isInstance(err) + const message = isFrontmatter ? err.data.message : `Failed to parse skill ${match}` + state.invalid.push({ + path: match, + reason: isFrontmatter ? "frontmatter" : "parse", + message, + }) const { Session } = yield* Effect.promise(() => import("@/session/session")) yield* bus.publish(Session.Event.Error, { error: new NamedError.Unknown({ message }).toObject() }) log.error("failed to load skill", { skill: match, err }) @@ -93,15 +114,50 @@ const add = Effect.fnUntraced(function* (state: State, match: string, bus: Bus.I if (!md) return - const parsed = z.object({ name: z.string(), description: z.string().optional() }).safeParse(md.data) - if (!parsed.success) return + const parsed = z + .object({ + name: z + .string() + .min(1, "Skill name is required") + .max(64, "Skill name must be at most 64 characters") + .regex(/^[a-z0-9-]+$/, "Skill name must contain only lowercase letters, numbers, and hyphens") + .regex(/^[^-].*[^-]$|^[^-]$/, "Skill name must not start or end with a hyphen") + .refine((value) => !value.includes("--"), "Skill name must not contain consecutive hyphens") + .refine((value) => !/(<[^>]*>)/.test(value), "Skill name must not contain XML tags") + .refine( + (value) => !value.includes("anthropic") && !value.includes("claude"), + 'Skill name must not contain reserved words "anthropic" or "claude"', + ) + .refine((value) => value === path.basename(path.dirname(match)), "Skill name must match parent directory name"), + description: z + .string({ error: "Skill description is required" }) + .trim() + .min(1, "Skill description is required") + .max(1024, "Skill description must be at most 1024 characters") + .refine((value) => !/(<[^>]*>)/.test(value), "Skill description must not contain XML tags"), + }) + .safeParse(md.data) + if (!parsed.success) { + state.invalid.push({ + path: match, + reason: "schema", + message: parsed.error.issues.map((issue) => issue.message).join(", "), + }) + return + } if (state.skills[parsed.data.name]) { + state.invalid.push({ + path: match, + reason: "duplicate", + message: `Duplicate skill name "${parsed.data.name}" already loaded from ${state.skills[parsed.data.name].location}`, + }) log.warn("duplicate skill name", { name: parsed.data.name, existing: state.skills[parsed.data.name].location, duplicate: match, }) + return } state.dirs.add(path.dirname(match)) @@ -229,7 +285,7 @@ export const layer = Layer.effect( ) const state = yield* InstanceState.make( Effect.fn("Skill.state")(function* () { - const s: State = { skills: {}, dirs: new Set() } + const s: State = { skills: {}, invalid: [], dirs: new Set() } yield* loadSkills(s, yield* InstanceState.get(discovered), bus) return s }), @@ -245,6 +301,10 @@ export const layer = Layer.effect( return Object.values(s.skills) }) + const invalid = Effect.fn("Skill.invalid")(function* () { + return (yield* InstanceState.get(state)).invalid + }) + const dirs = Effect.fn("Skill.dirs")(function* () { return (yield* InstanceState.get(discovered)).dirs }) @@ -256,7 +316,7 @@ export const layer = Layer.effect( return list.filter((skill) => Permission.evaluate("skill", skill.name, agent.permission).action !== "deny") }) - return Service.of({ get, all, dirs, available }) + return Service.of({ get, all, invalid, dirs, available }) }), ) diff --git a/packages/opencode/test/session/system.test.ts b/packages/opencode/test/session/system.test.ts index 1cf9026725..4dae6c8896 100644 --- a/packages/opencode/test/session/system.test.ts +++ b/packages/opencode/test/session/system.test.ts @@ -48,6 +48,7 @@ const it = testEffect( Skill.Service.of({ get: (name) => Effect.succeed(skills.find((skill) => skill.name === name)), all: () => Effect.succeed(skills), + invalid: () => Effect.succeed([]), dirs: () => Effect.succeed([]), available: () => Effect.succeed(skills), }), diff --git a/packages/opencode/test/skill/skill.test.ts b/packages/opencode/test/skill/skill.test.ts index d73750b083..e8ef3819f2 100644 --- a/packages/opencode/test/skill/skill.test.ts +++ b/packages/opencode/test/skill/skill.test.ts @@ -163,7 +163,7 @@ Just some content without YAML frontmatter. ), ) - it.live("discovers skills without descriptions", () => + it.live("tracks skills without descriptions as invalid", () => provideTmpdirInstance( (dir) => Effect.gen(function* () { @@ -183,10 +183,11 @@ Instructions here. const skill = yield* Skill.Service const list = yield* skill.all() - expect(list.length).toBe(1) - const item = list.find((x) => x.name === "manual-skill") - expect(item).toBeDefined() - expect(item!.description).toBeUndefined() + const invalid = yield* skill.invalid() + expect(list.length).toBe(0) + expect(invalid.length).toBe(1) + expect(invalid[0].reason).toBe("schema") + expect(invalid[0].message).toContain("Skill description is required") expect(Skill.fmt(list, { verbose: false })).toBe("No skills are currently available.") expect(Skill.fmt(list, { verbose: true })).toBe("No skills are currently available.") }), @@ -200,13 +201,13 @@ Instructions here. Effect.gen(function* () { yield* Effect.promise(() => Bun.write( - path.join(dir, ".claude", "skills", "claude-skill", "SKILL.md"), + path.join(dir, ".claude", "skills", "external-skill", "SKILL.md"), `--- -name: claude-skill +name: external-skill description: A skill in the .claude/skills directory. --- -# Claude Skill +# External Skill `, ), ) @@ -214,9 +215,9 @@ description: A skill in the .claude/skills directory. const skill = yield* Skill.Service const list = yield* skill.all() expect(list.length).toBe(1) - const item = list.find((x) => x.name === "claude-skill") + const item = list.find((x) => x.name === "external-skill") expect(item).toBeDefined() - expect(item!.location).toContain(path.join(".claude", "skills", "claude-skill", "SKILL.md")) + expect(item!.location).toContain(path.join(".claude", "skills", "external-skill", "SKILL.md")) }), { git: true }, ), @@ -332,13 +333,13 @@ This skill is loaded from the global home directory. yield* Effect.promise(() => Promise.all([ Bun.write( - path.join(dir, ".claude", "skills", "claude-skill", "SKILL.md"), + path.join(dir, ".claude", "skills", "external-skill", "SKILL.md"), `--- -name: claude-skill +name: external-skill description: A skill in the .claude/skills directory. --- -# Claude Skill +# External Skill `, ), Bun.write( @@ -357,13 +358,55 @@ description: A skill in the .agents/skills directory. const skill = yield* Skill.Service const list = yield* skill.all() expect(list.length).toBe(2) - expect(list.find((x) => x.name === "claude-skill")).toBeDefined() + expect(list.find((x) => x.name === "external-skill")).toBeDefined() expect(list.find((x) => x.name === "agent-skill")).toBeDefined() }), { git: true }, ), ) + it.live("tracks duplicate skill names as invalid diagnostics", () => + provideTmpdirInstance( + (dir) => + Effect.gen(function* () { + yield* Effect.promise(() => + Promise.all([ + Bun.write( + path.join(dir, ".agents", "skills", "duplicate-skill", "SKILL.md"), + `--- +name: duplicate-skill +description: A duplicate skill in the .agents directory. +--- + +# Duplicate Skill +`, + ), + Bun.write( + path.join(dir, ".claude", "skills", "duplicate-skill", "SKILL.md"), + `--- +name: duplicate-skill +description: A duplicate skill in the .claude directory. +--- + +# Duplicate Skill +`, + ), + ]), + ) + + const skill = yield* Skill.Service + const list = yield* skill.all() + const invalid = yield* skill.invalid() + expect(list.length).toBe(1) + expect(list[0].name).toBe("duplicate-skill") + expect(invalid.length).toBe(1) + expect(invalid[0].reason).toBe("duplicate") + expect(invalid[0].message).toContain("Duplicate skill name") + }), + { git: true }, + ), + ) + it.live("properly resolves directories that skills live in", () => provideTmpdirInstance( (dir) => diff --git a/packages/sdk/js/src/v2/gen/sdk.gen.ts b/packages/sdk/js/src/v2/gen/sdk.gen.ts index f25596011e..d49e38467d 100644 --- a/packages/sdk/js/src/v2/gen/sdk.gen.ts +++ b/packages/sdk/js/src/v2/gen/sdk.gen.ts @@ -407,6 +407,7 @@ export class App extends HeyApiClient { parameters?: { directory?: string workspace?: string + include?: "invalid" }, options?: Options, ) { @@ -417,6 +418,7 @@ export class App extends HeyApiClient { args: [ { in: "query", key: "directory" }, { in: "query", key: "workspace" }, + { in: "query", key: "include" }, ], }, ], diff --git a/packages/sdk/js/src/v2/gen/types.gen.ts b/packages/sdk/js/src/v2/gen/types.gen.ts index 220278b8c2..c69ee55cc0 100644 --- a/packages/sdk/js/src/v2/gen/types.gen.ts +++ b/packages/sdk/js/src/v2/gen/types.gen.ts @@ -4233,6 +4233,7 @@ export type AppSkillsData = { query?: { directory?: string workspace?: string + include?: "invalid" } url: "/skill" } @@ -4241,12 +4242,26 @@ export type AppSkillsResponses = { /** * List of skills */ - 200: Array<{ - name: string - description?: string - location: string - content: string - }> + 200: + | Array<{ + name: string + description?: string + location: string + content: string + }> + | { + skills: Array<{ + name: string + description?: string + location: string + content: string + }> + invalid: Array<{ + path: string + reason: "parse" | "frontmatter" | "schema" | "duplicate" + message: string + }> + } } export type AppSkillsResponse = AppSkillsResponses[keyof AppSkillsResponses]