From 7156b019e89b843ba3c9b13c9cb051ea73fa3f85 Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Thu, 18 Jun 2026 18:58:10 +0300 Subject: [PATCH 1/2] feat(skills): full agentskills.io spec compliance - escape catalog XML and drop file:// prefix on - skip skills missing a required description; add Skill.Validate - add license/compatibility/metadata/allowed-tools/disable-model-invocation frontmatter fields plus a malformed-YAML (unquoted colon) fallback - scan ~/.agents/skills and dedupe by name with project>user precedence - treat --skills-dir as a direct directory; add --skill-disable + DisableSkill/EnableSkill SDK methods - enumerate bundled resources via on activation - add activate_skill MCP tool with enum-constrained name and session dedup - protect activated skill content from compaction pruning - gate project-local skills on a persisted trust allowlist via SkillTrustPrompt and an interactive CLI prompt - document new fields, flags, and SDK surface across README and docs site Fixes #65 --- README.md | 14 +- cmd/root.go | 14 +- cmd/skill_trust.go | 52 ++++ internal/compaction/compaction.go | 32 ++ internal/compaction/compaction_test.go | 22 ++ internal/config/config.go | 17 +- internal/extensions/api.go | 20 +- internal/skills/prompt_builder_test.go | 2 +- internal/skills/skills.go | 390 ++++++++++++++++++++++--- internal/skills/skills_test.go | 224 ++++++++++++-- internal/skilltool/skilltool.go | 139 +++++++++ internal/skilltool/skilltool_test.go | 89 ++++++ internal/trust/trust.go | 153 ++++++++++ internal/trust/trust_test.go | 60 ++++ pkg/kit/kit.go | 91 +++++- pkg/kit/skills.go | 133 ++++++++- pkg/kit/skills_spec_test.go | 120 ++++++++ www/pages/cli/commands.md | 51 +++- www/pages/cli/flags.md | 3 +- www/pages/configuration.md | 3 +- www/pages/extensions/capabilities.md | 7 +- www/pages/sdk/options.md | 46 ++- www/pages/sdk/overview.md | 22 +- 23 files changed, 1597 insertions(+), 107 deletions(-) create mode 100644 cmd/skill_trust.go create mode 100644 internal/skilltool/skilltool.go create mode 100644 internal/skilltool/skilltool_test.go create mode 100644 internal/trust/trust.go create mode 100644 internal/trust/trust_test.go create mode 100644 pkg/kit/skills_spec_test.go diff --git a/README.md b/README.md index 9cb829cb..61fa3df0 100644 --- a/README.md +++ b/README.md @@ -130,11 +130,13 @@ stream: true thinking-level: off # off, none, minimal, low, medium, high no-core-tools: false # set to true to disable all built-in core tools -# Skills — all three keys are optional +# Skills — all keys are optional no-skills: false # set to true to disable all skill loading skill: # explicit skill files/dirs (disables auto-discovery) - /path/to/skill.md -skills-dir: "" # override project-local directory for auto-discovery +skills-dir: "" # scan this directory directly for skills (overrides auto-discovery) +skill-disable: # hide skills from the model catalog by name (still usable via /skill:) + - some-skill ``` All of the above keys can also be set programmatically via the SDK @@ -212,7 +214,8 @@ mcpServers: # Skills --skill Load skill file or directory (repeatable) ---skills-dir Override the project-local skills directory for auto-discovery +--skills-dir Scan this directory directly for skills (overrides auto-discovery) +--skill-disable Hide a skill from the model catalog by name (repeatable); still usable via /skill: --no-skills Disable skill loading (auto-discovery and explicit) # Generation parameters @@ -890,6 +893,11 @@ host.AddContextFileContent( host.RemoveSkill("polite-french") host.RemoveContextFile(fmt.Sprintf("session://%s/AGENTS.md", userID)) +// Hide a skill from the model catalog without unloading it (still usable +// via /skill:); EnableSkill reverses it. +host.DisableSkill("refund-policy") +host.EnableSkill("refund-policy") + // Or replace the whole set atomically. host.SetSkills(activeSkillsForUser) host.SetContextFiles(activeContextForUser) diff --git a/cmd/root.go b/cmd/root.go index cb303100..f1aa5daf 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -74,9 +74,10 @@ var ( extensionPaths []string // Skills control - noSkillsFlag bool - skillsPaths []string - skillsDir string + noSkillsFlag bool + skillsPaths []string + skillsDir string + skillsDisable []string // TLS configuration tlsSkipVerify bool @@ -294,7 +295,9 @@ func init() { rootCmd.PersistentFlags(). StringSliceVar(&skillsPaths, "skill", nil, "load skill file or directory (repeatable)") rootCmd.PersistentFlags(). - StringVar(&skillsDir, "skills-dir", "", "override the project-local skills directory for auto-discovery") + StringVar(&skillsDir, "skills-dir", "", "scan this directory directly for skills (overrides auto-discovery)") + rootCmd.PersistentFlags(). + StringSliceVar(&skillsDisable, "skill-disable", nil, "hide a skill from the model catalog by name (repeatable); still usable via /skill:") flags := rootCmd.PersistentFlags() flags.StringVar(&providerURL, "provider-url", "", "base URL for the provider API (applies to OpenAI, Anthropic, Ollama, and Google)") @@ -349,6 +352,7 @@ func init() { _ = viper.BindPFlag("no-skills", rootCmd.PersistentFlags().Lookup("no-skills")) _ = viper.BindPFlag("skill", rootCmd.PersistentFlags().Lookup("skill")) _ = viper.BindPFlag("skills-dir", rootCmd.PersistentFlags().Lookup("skills-dir")) + _ = viper.BindPFlag("skill-disable", rootCmd.PersistentFlags().Lookup("skill-disable")) // Defaults are already set in flag definitions, no need to duplicate in viper @@ -842,6 +846,8 @@ func runNormalMode(ctx context.Context) error { NoSkills: noSkillsFlag, Skills: skillsPaths, SkillsDir: skillsDir, + SkillsDisable: skillsDisable, + SkillTrustPrompt: skillTrustPrompt(), // This callback is called when each MCP server finishes loading. // We use a closure that captures appInstancePtr which is set after // app.New() is called below. diff --git a/cmd/skill_trust.go b/cmd/skill_trust.go new file mode 100644 index 00000000..36028e3b --- /dev/null +++ b/cmd/skill_trust.go @@ -0,0 +1,52 @@ +package cmd + +import ( + "bufio" + "fmt" + "os" + "strings" + + "golang.org/x/term" + + "github.com/mark3labs/kit/pkg/kit" +) + +// skillTrustPrompt returns a callback that gates project-local skill loading +// on an interactive trust decision (issue #65, gap #8). Project-local skills +// are injected into the system prompt, so a freshly cloned untrusted repo +// could smuggle instructions into the agent. The prompt asks the user whether +// to trust the directory before any project skill is loaded. +// +// It returns nil — meaning "load without prompting" — when Kit is not running +// interactively (a non-TTY stdin, --quiet, or a non-interactive one-shot +// prompt), so scripted and piped invocations keep their existing behaviour. +func skillTrustPrompt() func(projectDir string, skillCount int) kit.TrustDecision { + // Only prompt for interactive terminal sessions. + if quietFlag || positionalPrompt != "" { + return nil + } + if !term.IsTerminal(int(os.Stdin.Fd())) { + return nil + } + + return func(projectDir string, skillCount int) kit.TrustDecision { + noun := "skills" + if skillCount == 1 { + noun = "skill" + } + fmt.Printf("\nThis project provides %d %s under .agents/skills or .kit/skills:\n %s\n", + skillCount, noun, projectDir) + fmt.Print("Load them into the agent? [t]rust always / [o]nce / [s]kip (default skip): ") + + reader := bufio.NewReader(os.Stdin) + line, _ := reader.ReadString('\n') + switch strings.ToLower(strings.TrimSpace(line)) { + case "t", "trust", "a", "always": + return kit.TrustProject + case "o", "once", "y", "yes": + return kit.TrustProjectOnce + default: + return kit.SkipProjectSkills + } + } +} diff --git a/internal/compaction/compaction.go b/internal/compaction/compaction.go index 71d15005..1c9d263f 100644 --- a/internal/compaction/compaction.go +++ b/internal/compaction/compaction.go @@ -389,6 +389,30 @@ func roleLabel(role fantasy.MessageRole) string { } } +// skillContentMarkers are substrings that identify a message carrying +// explicitly-activated skill content. Such messages are exempt from +// compaction pruning per the agentskills.io spec (issue #65, gap #7): an +// activated skill must remain in context verbatim instead of being folded +// into a lossy summary. +var skillContentMarkers = []string{"", "body`, true}, + {`body`, true}, + {"just a normal message", false}, + {"talking about skills in general", false}, + } + for _, c := range cases { + msg := makeTextMessage(fantasy.MessageRoleUser, c.text) + if got := isProtectedMessage(msg); got != c.want { + t.Errorf("isProtectedMessage(%q) = %v, want %v", c.text, got, c.want) + } + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 2fdf17f6..511d74f7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -499,7 +499,22 @@ mcpServers: # no-skills: false # Set to true to disable all skill loading # skill: # Explicit skill files/dirs (disables auto-discovery) # - "/path/to/skill.md" -# skills-dir: "/path/to/skills" # Override project-local directory for auto-discovery +# skills-dir: "/path/to/skills" # Scan this directory directly for skills (overrides auto-discovery) +# skill-disable: # Hide skills from the model catalog by name (still usable via /skill:) +# - "some-skill" +# +# Skill files follow the agentskills.io spec. A SKILL.md frontmatter block +# supports these fields: +# name: my-skill # required +# description: Use when ... # required (basis for model discovery) +# license: MIT # optional SPDX identifier +# compatibility: claude-code, cursor # optional targeted-environment note +# allowed-tools: read, bash # optional (experimental) tool restriction +# disable-model-invocation: false # optional; true hides from the catalog +# metadata: # optional arbitrary key/value pairs +# author: you +# tags: [example] # Kit extension +# when: on-demand # Kit extension # API Configuration (can also use environment variables) # provider-api-key: "your-api-key" # API key for OpenAI, Anthropic, or Google diff --git a/internal/extensions/api.go b/internal/extensions/api.go index f01636e1..1ff18c7c 100644 --- a/internal/extensions/api.go +++ b/internal/extensions/api.go @@ -777,7 +777,8 @@ type Context struct { LoadSkillsFromDir func(dir string) SkillLoadResult // DiscoverSkills finds skills in standard locations. - // Checks ~/.config/kit/skills/, .kit/skills/, .agents/skills/ + // Checks ~/.agents/skills/, ~/.config/kit/skills/, /.agents/skills/, + // and /.kit/skills/. DiscoverSkills func() SkillLoadResult // InjectSkillAsContext sends a skill's content as a system message. @@ -909,9 +910,24 @@ type Skill struct { Content string // Path is the absolute filesystem path. Path string - // Tags are optional labels for categorization. + // License is an optional SPDX license identifier (agentskills.io field). + License string + // Compatibility is an optional note describing targeted environments + // (agentskills.io field). + Compatibility string + // Metadata is an optional bag of arbitrary string key/value pairs + // (agentskills.io field). + Metadata map[string]string + // AllowedTools optionally restricts which tools the skill may use + // (experimental agentskills.io field). + AllowedTools string + // DisableModelInvocation hides the skill from the model-facing catalog + // while keeping it available via explicit activation (agentskills.io field). + DisableModelInvocation bool + // Tags are optional labels for categorization. Kit extension. Tags []string // When controls automatic inclusion: "always", "on-demand", or file-glob. + // Kit extension. When string } diff --git a/internal/skills/prompt_builder_test.go b/internal/skills/prompt_builder_test.go index 396e96ae..1ddb0c40 100644 --- a/internal/skills/prompt_builder_test.go +++ b/internal/skills/prompt_builder_test.go @@ -50,7 +50,7 @@ func TestPromptBuilder_WithSkills(t *testing.T) { if !strings.Contains(result, "Write code") { t.Error("missing skill description in XML") } - if !strings.Contains(result, "file:///tmp/coding/SKILL.md") { + if !strings.Contains(result, "/tmp/coding/SKILL.md") { t.Error("missing skill location") } } diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 61a135d9..1d00275a 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -2,43 +2,173 @@ // // Skills are markdown instruction files with optional YAML frontmatter that // provide domain-specific context, instructions, and workflows to the agent. -// They follow a hierarchical discovery pattern similar to extensions: +// They follow the cross-client agentskills.io discovery convention plus a +// Kit-native location: // -// ~/.config/kit/skills/ global skills directory -// .kit/skills/ project-local skills directory +// ~/.agents/skills/ user-level cross-client skills +// ~/.config/kit/skills/ user-level Kit skills ($XDG_CONFIG_HOME aware) +// /.agents/skills/ project-local cross-client skills +// /.kit/skills/ project-local Kit skills // -// Skills can be single .md/.txt files or subdirectories containing a SKILL.md file. +// Skills can be single .md/.txt files or subdirectories containing a SKILL.md +// file. Project-level skills take precedence over user-level skills when two +// skills share the same name. package skills import ( "bytes" + "encoding/xml" "errors" "fmt" "io/fs" "os" "path" "path/filepath" + "regexp" + "sort" "strings" + "github.com/charmbracelet/log" "gopkg.in/yaml.v3" ) // Skill represents a markdown-based instruction file that provides // domain-specific context and workflows to the agent. +// +// The Name and Description fields are required by the agentskills.io +// specification. License, Compatibility, Metadata, and AllowedTools are +// optional spec fields. Tags and When are Kit-specific extensions that other +// clients ignore. type Skill struct { - // Name is the human-readable identifier for this skill. + // Name is the human-readable identifier for this skill. Required. Name string `yaml:"name" json:"name"` - // Description summarises what this skill provides. + // Description summarises what this skill provides and when to use it. + // Required by the spec — it is the sole basis on which the model decides + // whether a skill is relevant, so a skill without one is omitted from the + // catalog. Description string `yaml:"description" json:"description"` // Content is the full markdown body (after frontmatter). Content string `yaml:"-" json:"content"` // Path is the absolute filesystem path the skill was loaded from. Path string `yaml:"-" json:"path"` - // Tags are optional labels for categorisation. + + // License is an optional SPDX license identifier (spec field). + License string `yaml:"license,omitempty" json:"license,omitempty"` + // Compatibility is an optional free-form note describing the environments + // or clients the skill targets (spec field). The model can use it to adapt + // execution. + Compatibility string `yaml:"compatibility,omitempty" json:"compatibility,omitempty"` + // Metadata is an optional bag of arbitrary string key/value pairs (spec + // field) for client-specific annotations. + Metadata map[string]string `yaml:"metadata,omitempty" json:"metadata,omitempty"` + // AllowedTools optionally restricts which tools the skill may use. This is + // an experimental spec field carried for portability; Kit does not yet + // enforce it. + AllowedTools string `yaml:"allowed-tools,omitempty" json:"allowed_tools,omitempty"` + // DisableModelInvocation, when true, hides the skill from the + // model-facing catalog (spec field). The skill can still be activated + // explicitly via the /skill: slash command. + DisableModelInvocation bool `yaml:"disable-model-invocation,omitempty" json:"disable_model_invocation,omitempty"` + + // Tags are optional labels for categorisation. Kit extension. Tags []string `yaml:"tags,omitempty" json:"tags,omitempty"` // When controls automatic inclusion: "always", "on-demand", or a - // file-glob like "file:*.go". Empty defaults to "on-demand". + // file-glob like "file:*.go". Empty defaults to "on-demand". Kit extension. When string `yaml:"when,omitempty" json:"when,omitempty"` + + // project records whether the skill was discovered in a project-local + // scope. Used internally for name-collision precedence (project > user). + project bool `yaml:"-" json:"-"` +} + +// Diagnostic describes a validation problem with a skill. Severity is either +// "error" (the skill cannot be used) or "warning" (the skill is usable but +// non-compliant). +type Diagnostic struct { + // Severity is "error" or "warning". + Severity string `json:"severity"` + // Field names the frontmatter field the diagnostic relates to, if any. + Field string `json:"field,omitempty"` + // Message is a human-readable description of the problem. + Message string `json:"message"` +} + +// Validate checks the skill against the agentskills.io specification and +// returns a list of diagnostics. An empty slice means the skill is fully +// compliant. A missing description is reported as an error because the spec +// makes it required for discovery. +func (s *Skill) Validate() []Diagnostic { + var diags []Diagnostic + if strings.TrimSpace(s.Name) == "" { + diags = append(diags, Diagnostic{Severity: "error", Field: "name", Message: "name is required"}) + } + if strings.TrimSpace(s.Description) == "" { + diags = append(diags, Diagnostic{ + Severity: "error", + Field: "description", + Message: "description is required for skill discovery", + }) + } + return diags +} + +// hasError reports whether diags contains a diagnostic with "error" severity. +func hasError(diags []Diagnostic) bool { + for _, d := range diags { + if d.Severity == "error" { + return true + } + } + return false +} + +// BaseDir returns the directory the skill was loaded from. Relative resources +// referenced by a skill (scripts/, references/, assets/) resolve against this +// directory. +func (s *Skill) BaseDir() string { + if s.Path == "" { + return "" + } + return filepath.Dir(s.Path) +} + +// resourceDirs are the conventional subdirectories a skill may bundle. +var resourceDirs = []string{"scripts", "references", "assets"} + +// maxResources caps how many bundled resources are enumerated to avoid +// flooding the prompt for skills with large asset trees. +const maxResources = 50 + +// Resources walks one level into the skill's scripts/, references/, and +// assets/ subdirectories and returns the relative paths of any files found +// (slash-separated, relative to BaseDir). The result is capped at 50 entries. +// It returns nil when the skill has no bundled resources or its Path is not a +// real on-disk file. +func (s *Skill) Resources() []string { + base := s.BaseDir() + if base == "" { + return nil + } + var out []string + for _, sub := range resourceDirs { + dir := filepath.Join(base, sub) + entries, err := os.ReadDir(dir) + if err != nil { + continue + } + for _, e := range entries { + if e.IsDir() { + continue + } + out = append(out, sub+"/"+e.Name()) + if len(out) >= maxResources { + sort.Strings(out) + return out + } + } + } + sort.Strings(out) + return out } // frontmatterSep is the YAML frontmatter delimiter. @@ -79,7 +209,7 @@ func parseSkill(data []byte, srcPath, storePath string) (*Skill, error) { // Strip an optional trailing newline right after the closing ---. body = strings.TrimPrefix(body, "\n") - if err := yaml.Unmarshal([]byte(frontmatter), skill); err != nil { + if err := unmarshalFrontmatter([]byte(frontmatter), skill); err != nil { return nil, fmt.Errorf("parsing frontmatter in %s: %w", srcPath, err) } skill.Content = strings.TrimSpace(body) @@ -105,6 +235,57 @@ func parseSkill(data []byte, srcPath, storePath string) (*Skill, error) { return skill, nil } +// unquotedColonRe matches a YAML scalar line whose value contains an unquoted +// colon, e.g. `description: Use when: extracting tables`. This is the most +// common frontmatter authoring mistake in cross-client skills and breaks +// strict YAML parsing. +var unquotedColonRe = regexp.MustCompile(`^(\s*[A-Za-z0-9_-]+):[ \t]+([^'"\n].*:.*)$`) + +// unmarshalFrontmatter unmarshals YAML frontmatter into skill, tolerating the +// common "unquoted colon in a scalar value" mistake (e.g. +// `description: Use when: …`). On a parse failure it quotes offending scalar +// values and retries once before giving up. +func unmarshalFrontmatter(frontmatter []byte, skill *Skill) error { + err := yaml.Unmarshal(frontmatter, skill) + if err == nil { + return nil + } + + // Attempt a single recovery pass: quote scalar values that contain an + // unquoted colon, which is the dominant cross-client failure mode. + repaired, changed := repairUnquotedColons(string(frontmatter)) + if !changed { + return err + } + if retryErr := yaml.Unmarshal([]byte(repaired), skill); retryErr != nil { + // The original error is more useful to the author. + return err + } + return nil +} + +// repairUnquotedColons quotes scalar values containing an unquoted colon and +// reports whether any line was changed. +func repairUnquotedColons(frontmatter string) (string, bool) { + lines := strings.Split(frontmatter, "\n") + changed := false + for i, line := range lines { + m := unquotedColonRe.FindStringSubmatch(line) + if m == nil { + continue + } + key, value := m[1], strings.TrimRight(m[2], " \t") + // Escape embedded double quotes before wrapping. + value = strings.ReplaceAll(value, `"`, `\"`) + lines[i] = fmt.Sprintf(`%s: "%s"`, key, value) + changed = true + } + if !changed { + return frontmatter, false + } + return strings.Join(lines, "\n"), true +} + // LoadSkillsFromDir loads all skills from a single directory. It looks for: // - *.md and *.txt files directly in dir // - SKILL.md (case-insensitive) in immediate subdirectories @@ -223,54 +404,127 @@ func LoadSkillsFromFS(fsys fs.FS, root string) ([]*Skill, error) { return skills, nil } -// LoadSkills auto-discovers skills from standard directories: -// 1. Global: $XDG_CONFIG_HOME/kit/skills/ (default ~/.config/kit/skills/) -// 2. Project-local: /.kit/skills/ +// LoadUserSkills discovers skills from the user-level scopes only: // -// Skills from project-local directories take precedence (appended last). -// cwd is the working directory for project-local discovery; if empty the -// current working directory is used. -func LoadSkills(cwd string) ([]*Skill, error) { +// 1. ~/.agents/skills/ (cross-client convention) +// 2. $XDG_CONFIG_HOME/kit/skills/ (default ~/.config/kit/skills/) +// +// The returned skills are not yet validated or deduplicated; pass them through +// Combine together with project skills to produce the final catalog set. +func LoadUserSkills() []*Skill { + var loaded []*Skill + if home, err := os.UserHomeDir(); err == nil && home != "" { + ss, _ := LoadSkillsFromDir(filepath.Join(home, ".agents", "skills")) + loaded = append(loaded, ss...) + } + if g := globalSkillsDir(); g != "" { + ss, _ := LoadSkillsFromDir(g) + loaded = append(loaded, ss...) + } + for _, s := range loaded { + s.project = false + } + return loaded +} + +// LoadProjectSkills discovers skills from the project-local scopes only: +// +// 1. /.agents/skills/ (cross-client convention) +// 2. /.kit/skills/ (Kit-specific) +// +// Because project-local skills are injected into the system prompt, callers +// may wish to gate this on a trust check before including the result. The +// returned skills are not yet validated or deduplicated; pass them through +// Combine. +func LoadProjectSkills(cwd string) []*Skill { if cwd == "" { cwd, _ = os.Getwd() } + var loaded []*Skill + for _, dir := range []string{ + filepath.Join(cwd, ".agents", "skills"), + filepath.Join(cwd, ".kit", "skills"), + } { + ss, _ := LoadSkillsFromDir(dir) + loaded = append(loaded, ss...) + } + for _, s := range loaded { + s.project = true + } + return loaded +} - seen := make(map[string]bool) - var all []*Skill +// Combine validates and deduplicates the union of user-level and project-level +// skills. Skills missing a required description are skipped with a logged +// warning; when two skills share a Name the project-level one wins (also +// logged). User skills are considered before project skills so first-seen +// ordering is stable. +func Combine(user, project []*Skill) []*Skill { + combined := make([]*Skill, 0, len(user)+len(project)) + combined = append(combined, user...) + combined = append(combined, project...) + return finalizeSkills(combined) +} - addUnique := func(skills []*Skill) { - for _, s := range skills { - if !seen[s.Path] { - seen[s.Path] = true - all = append(all, s) +// LoadSkills auto-discovers skills from the standard agentskills.io scopes: +// +// 1. User-level: ~/.agents/skills/ (cross-client convention) +// 2. User-level: $XDG_CONFIG_HOME/kit/skills/ (default ~/.config/kit/skills/) +// 3. Project-local: /.agents/skills/ (cross-client convention) +// 4. Project-local: /.kit/skills/ (Kit-specific) +// +// When two skills share the same Name, the project-level skill takes +// precedence over a user-level one and a warning is logged. cwd is the working +// directory for project-local discovery; if empty the current working +// directory is used. +func LoadSkills(cwd string) ([]*Skill, error) { + return Combine(LoadUserSkills(), LoadProjectSkills(cwd)), nil +} + +// finalizeSkills applies validation (skipping skills missing a required +// description) and name-collision precedence (project overrides user). It +// preserves first-seen ordering for stable catalog output. +func finalizeSkills(loaded []*Skill) []*Skill { + byName := make(map[string]int) // name → index in result + var result []*Skill + + for _, s := range loaded { + if diags := s.Validate(); hasError(diags) { + for _, d := range diags { + if d.Severity == "error" { + log.Warn("skipping skill: validation failed", "path", s.Path, "field", d.Field, "reason", d.Message) + } } + continue } - } - - // Global skills. - globalDir := globalSkillsDir() - if globalDir != "" { - global, _ := LoadSkillsFromDir(globalDir) - addUnique(global) - } - // Project-local skills: .agents/skills/ (standardized cross-tool convention). - agentsDir := filepath.Join(cwd, ".agents", "skills") - agentsSkills, _ := LoadSkillsFromDir(agentsDir) - addUnique(agentsSkills) + if idx, ok := byName[s.Name]; ok { + existing := result[idx] + // Project-level skills override user-level skills. + if s.project && !existing.project { + log.Warn("skill name collision: project skill overrides user skill", + "name", s.Name, "project", s.Path, "user", existing.Path) + result[idx] = s + } else { + log.Warn("skill name collision: keeping earlier skill, ignoring duplicate", + "name", s.Name, "kept", existing.Path, "ignored", s.Path) + } + continue + } - // Project-local skills: .kit/skills/ (kit-specific). - localDir := filepath.Join(cwd, ".kit", "skills") - local, _ := LoadSkillsFromDir(localDir) - addUnique(local) + byName[s.Name] = len(result) + result = append(result, s) + } - return all, nil + return result } // FormatForPrompt formats skills as metadata-only XML for inclusion in a // system prompt. Only the name, description, and file location are included; -// the agent reads the full skill file on demand using the read tool. This - +// the agent reads the full skill file on demand using the read tool. Skill +// fields are XML-escaped so that descriptions containing <, >, & or quotes +// produce valid markup. Skills with DisableModelInvocation set are omitted +// from the catalog (they remain available via the /skill: slash command). func FormatForPrompt(skills []*Skill) string { if len(skills) == 0 { return "" @@ -282,17 +536,63 @@ func FormatForPrompt(skills []*Skill) string { buf.WriteString("When a skill file references a relative path, resolve it against the skill directory (parent of SKILL.md) and use that absolute path in tool commands.\n") buf.WriteString("\n\n") + emitted := 0 for _, s := range skills { + if s.DisableModelInvocation { + continue + } buf.WriteString(" \n") - fmt.Fprintf(&buf, " %s\n", s.Name) + fmt.Fprintf(&buf, " %s\n", escapeXML(s.Name)) if s.Description != "" { - fmt.Fprintf(&buf, " %s\n", s.Description) + fmt.Fprintf(&buf, " %s\n", escapeXML(s.Description)) + } + if s.Compatibility != "" { + fmt.Fprintf(&buf, " %s\n", escapeXML(s.Compatibility)) } - fmt.Fprintf(&buf, " file://%s\n", s.Path) + fmt.Fprintf(&buf, " %s\n", escapeXML(s.Path)) buf.WriteString(" \n") + emitted++ } buf.WriteString("") + if emitted == 0 { + return "" + } + return buf.String() +} + +// escapeXML escapes a string for safe inclusion as XML text content. +func escapeXML(s string) string { + var buf bytes.Buffer + if err := xml.EscapeText(&buf, []byte(s)); err != nil { + return s + } + return buf.String() +} + +// FormatResources renders a skill's bundled resources as a +// block, or returns the empty string when the skill bundles no resources. It +// is used when a skill is explicitly activated so the model knows which files +// it can read without enumerating them itself. +func FormatResources(resources []string) string { + if len(resources) == 0 { + return "" + } + var buf bytes.Buffer + buf.WriteString("\n") + limit := len(resources) + truncated := false + if limit > maxResources { + limit = maxResources + truncated = true + } + for _, r := range resources[:limit] { + fmt.Fprintf(&buf, " %s\n", escapeXML(r)) + } + if truncated { + buf.WriteString(" \n") + } + buf.WriteString("") return buf.String() } diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index 22f4e151..7baee069 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -243,15 +243,24 @@ func TestLoadSkillsFromDir_CaseInsensitiveSKILLmd(t *testing.T) { // LoadSkills (auto-discovery) // --------------------------------------------------------------------------- +func writeSkill(t *testing.T, path, name, desc, body string) { + t.Helper() + content := "---\nname: " + name + "\ndescription: " + desc + "\n---\n" + body + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + func TestLoadSkills_ProjectLocal(t *testing.T) { dir := t.TempDir() + // Isolate user-level scopes so the host machine's skills don't leak in. + t.Setenv("XDG_CONFIG_HOME", filepath.Join(dir, "xdg")) + t.Setenv("HOME", filepath.Join(dir, "home")) skillsDir := filepath.Join(dir, ".kit", "skills") if err := os.MkdirAll(skillsDir, 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(skillsDir, "local.md"), []byte("Local skill"), 0o644); err != nil { - t.Fatal(err) - } + writeSkill(t, filepath.Join(skillsDir, "local.md"), "local", "A local skill", "Local skill") skills, err := LoadSkills(dir) if err != nil { @@ -265,37 +274,64 @@ func TestLoadSkills_ProjectLocal(t *testing.T) { } } -func TestLoadSkills_Deduplication(t *testing.T) { +// TestLoadSkills_SkipsMissingDescription verifies that a skill without a +// required description is skipped during auto-discovery (gap #2). +func TestLoadSkills_SkipsMissingDescription(t *testing.T) { dir := t.TempDir() - - // Set XDG_CONFIG_HOME to our temp dir so global and local overlap. - t.Setenv("XDG_CONFIG_HOME", dir) - - globalDir := filepath.Join(dir, "kit", "skills") - localDir := filepath.Join(dir, ".kit", "skills") - - if err := os.MkdirAll(globalDir, 0o755); err != nil { + t.Setenv("XDG_CONFIG_HOME", filepath.Join(dir, "xdg")) + t.Setenv("HOME", filepath.Join(dir, "home")) + skillsDir := filepath.Join(dir, ".kit", "skills") + if err := os.MkdirAll(skillsDir, 0o755); err != nil { + t.Fatal(err) + } + // No description — should be skipped. + if err := os.WriteFile(filepath.Join(skillsDir, "nodesc.md"), []byte("Just a body"), 0o644); err != nil { t.Fatal(err) } - if err := os.MkdirAll(localDir, 0o755); err != nil { + writeSkill(t, filepath.Join(skillsDir, "good.md"), "good", "Has a description", "Body") + + skills, err := LoadSkills(dir) + if err != nil { t.Fatal(err) } + if len(skills) != 1 { + t.Fatalf("expected 1 skill (missing-description skipped), got %d", len(skills)) + } + if skills[0].Name != "good" { + t.Errorf("Name = %q, want %q", skills[0].Name, "good") + } +} + +// TestLoadSkills_NameCollisionPrecedence verifies project-level skills override +// user-level skills with the same name (gap #5). +func TestLoadSkills_NameCollisionPrecedence(t *testing.T) { + dir := t.TempDir() - // Same content in both directories but different paths — both should load. - if err := os.WriteFile(filepath.Join(globalDir, "shared.md"), []byte("Global version"), 0o644); err != nil { + // Set XDG_CONFIG_HOME so the user-level skill lives under our temp dir. + t.Setenv("XDG_CONFIG_HOME", dir) + t.Setenv("HOME", filepath.Join(dir, "home")) + + userDir := filepath.Join(dir, "kit", "skills") + projectDir := filepath.Join(dir, ".kit", "skills") + if err := os.MkdirAll(userDir, 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(localDir, "shared.md"), []byte("Local version"), 0o644); err != nil { + if err := os.MkdirAll(projectDir, 0o755); err != nil { t.Fatal(err) } + writeSkill(t, filepath.Join(userDir, "shared.md"), "shared", "User version", "USER") + writeSkill(t, filepath.Join(projectDir, "shared.md"), "shared", "Project version", "PROJECT") + skills, err := LoadSkills(dir) if err != nil { t.Fatal(err) } - // Different absolute paths = both loaded. - if len(skills) != 2 { - t.Fatalf("expected 2 skills (different paths), got %d", len(skills)) + if len(skills) != 1 { + t.Fatalf("expected 1 skill (deduped by name), got %d", len(skills)) + } + if !strings.Contains(skills[0].Content, "PROJECT") { + t.Errorf("expected project version to win, got content %q", skills[0].Content) } } @@ -321,8 +357,8 @@ func TestFormatForPrompt_SingleSkill(t *testing.T) { if !strings.Contains(result, "A test") { t.Errorf("result should contain description in XML") } - if !strings.Contains(result, "file:///tmp/test-skill/SKILL.md") { - t.Errorf("result should contain file location") + if !strings.Contains(result, "/tmp/test-skill/SKILL.md") { + t.Errorf("result should contain bare file location (no file:// prefix)") } if !strings.Contains(result, "") { t.Errorf("result should contain available_skills root element") @@ -352,3 +388,149 @@ func TestFormatForPrompt_MultipleSkills(t *testing.T) { t.Error("missing preamble instructions") } } + +// --------------------------------------------------------------------------- +// agentskills.io spec compliance (issue #65) +// --------------------------------------------------------------------------- + +// TestFormatForPrompt_XMLEscaping verifies special characters in name and +// description are escaped so the catalog is valid XML (gap #1). +func TestFormatForPrompt_XMLEscaping(t *testing.T) { + skills := []*Skill{ + {Name: "a&b", Description: "use when & \"quoted\"", Path: "/tmp/x"}, + } + result := FormatForPrompt(skills) + if strings.Contains(result, "") { + t.Errorf("raw < should have been escaped, got: %q", result) + } + if !strings.Contains(result, "<tag>") { + t.Errorf("expected escaped , got: %q", result) + } + if !strings.Contains(result, "a&b") { + t.Errorf("expected escaped ampersand in name, got: %q", result) + } +} + +// TestFormatForPrompt_DisableModelInvocation verifies that a skill flagged +// disable-model-invocation is omitted from the catalog (gap #10). +func TestFormatForPrompt_DisableModelInvocation(t *testing.T) { + skills := []*Skill{ + {Name: "visible", Description: "shown", Path: "/tmp/a"}, + {Name: "hidden", Description: "not shown", Path: "/tmp/b", DisableModelInvocation: true}, + } + result := FormatForPrompt(skills) + if !strings.Contains(result, "visible") { + t.Error("visible skill should be in catalog") + } + if strings.Contains(result, "hidden") { + t.Error("disable-model-invocation skill should be omitted from catalog") + } +} + +// TestLoadSkill_NewSpecFields verifies the new frontmatter fields parse (gap #6). +func TestLoadSkill_NewSpecFields(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "spec.md") + content := `--- +name: spec-skill +description: A spec-compliant skill +license: MIT +compatibility: claude-code, cursor +allowed-tools: read, bash +disable-model-invocation: true +metadata: + author: jane + version: "1.2" +--- +Body.` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + s, err := LoadSkill(path) + if err != nil { + t.Fatal(err) + } + if s.License != "MIT" { + t.Errorf("License = %q, want MIT", s.License) + } + if s.Compatibility != "claude-code, cursor" { + t.Errorf("Compatibility = %q", s.Compatibility) + } + if s.AllowedTools != "read, bash" { + t.Errorf("AllowedTools = %q", s.AllowedTools) + } + if !s.DisableModelInvocation { + t.Error("DisableModelInvocation should be true") + } + if s.Metadata["author"] != "jane" || s.Metadata["version"] != "1.2" { + t.Errorf("Metadata = %v", s.Metadata) + } +} + +// TestLoadSkill_UnquotedColonFallback verifies the YAML repair fallback for +// the common `description: Use when: ...` mistake (gap #9). +func TestLoadSkill_UnquotedColonFallback(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "colon.md") + content := "---\nname: colon-skill\ndescription: Use when: extracting tables from PDFs\n---\nBody." + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + s, err := LoadSkill(path) + if err != nil { + t.Fatalf("expected unquoted-colon fallback to succeed, got error: %v", err) + } + if s.Name != "colon-skill" { + t.Errorf("Name = %q", s.Name) + } + if !strings.Contains(s.Description, "extracting tables") { + t.Errorf("Description = %q", s.Description) + } +} + +// TestValidate verifies the Validate diagnostics (gaps #2, #15). +func TestValidate(t *testing.T) { + missing := &Skill{Name: "x"} + diags := missing.Validate() + if !hasError(diags) { + t.Error("expected an error diagnostic for missing description") + } + + ok := &Skill{Name: "x", Description: "y"} + if len(ok.Validate()) != 0 { + t.Error("expected no diagnostics for a complete skill") + } +} + +// TestSkillResources verifies bundled-resource enumeration (gaps #11, #15). +func TestSkillResources(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "my-skill") + if err := os.MkdirAll(filepath.Join(skillDir, "scripts"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(skillDir, "references"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "scripts", "run.py"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "references", "REF.md"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + s := &Skill{Name: "my-skill", Path: filepath.Join(skillDir, "SKILL.md")} + res := s.Resources() + if len(res) != 2 { + t.Fatalf("expected 2 resources, got %d: %v", len(res), res) + } + if s.BaseDir() != skillDir { + t.Errorf("BaseDir = %q, want %q", s.BaseDir(), skillDir) + } + formatted := FormatResources(res) + if !strings.Contains(formatted, "references/REF.md") { + t.Errorf("FormatResources output missing reference: %q", formatted) + } + if !strings.Contains(formatted, "scripts/run.py") { + t.Errorf("FormatResources output missing script: %q", formatted) + } +} diff --git a/internal/skilltool/skilltool.go b/internal/skilltool/skilltool.go new file mode 100644 index 00000000..08124172 --- /dev/null +++ b/internal/skilltool/skilltool.go @@ -0,0 +1,139 @@ +// Package skilltool provides the built-in activate_skill tool, a dedicated +// activation entry point for agentskills.io skills (issue #65, gaps #13/#14). +// +// While a skill can always be activated by reading its SKILL.md with the +// generic read tool, a dedicated tool offers an enum-constrained skill name +// (preventing hallucinated names), bundled-resource enumeration, and +// per-session deduplication so the same skill is not re-injected repeatedly. +package skilltool + +import ( + "context" + "encoding/json" + "fmt" + "sort" + "strings" + "sync" + + "charm.land/fantasy" + + "github.com/mark3labs/kit/internal/skills" +) + +// SkillProvider returns the skills currently available for activation. It is +// queried on every call so runtime skill mutations are reflected. +type SkillProvider func() []*skills.Skill + +type activateArgs struct { + Name string `json:"name"` +} + +// activateSkillTool implements fantasy.AgentTool. +type activateSkillTool struct { + info fantasy.ToolInfo + provider SkillProvider + providerOptions fantasy.ProviderOptions + + mu sync.Mutex + activated map[string]bool // session-level dedup tracking +} + +func (t *activateSkillTool) Info() fantasy.ToolInfo { return t.info } +func (t *activateSkillTool) ProviderOptions() fantasy.ProviderOptions { return t.providerOptions } +func (t *activateSkillTool) SetProviderOptions(opts fantasy.ProviderOptions) { + t.providerOptions = opts +} + +// New builds the activate_skill tool. names is the initial set of skill names +// used to populate the enum constraint on the name parameter; provider is +// queried at call time to resolve the skill by name (so runtime additions +// resolve even if absent from the enum). Returns nil when no skill names are +// available. +func New(names []string, provider SkillProvider) fantasy.AgentTool { + if len(names) == 0 || provider == nil { + return nil + } + sorted := append([]string(nil), names...) + sort.Strings(sorted) + enum := make([]any, len(sorted)) + for i, n := range sorted { + enum[i] = n + } + + return &activateSkillTool{ + info: fantasy.ToolInfo{ + Name: "activate_skill", + Description: "Activate a skill by name to load its full instructions into context. " + + "Use this when a task matches a skill listed in . " + + "The skill body and a list of its bundled resources are returned.", + Parameters: map[string]any{ + "name": map[string]any{ + "type": "string", + "description": "The exact name of the skill to activate.", + "enum": enum, + }, + }, + Required: []string{"name"}, + Parallel: false, + }, + provider: provider, + activated: map[string]bool{}, + } +} + +func (t *activateSkillTool) Run(_ context.Context, call fantasy.ToolCall) (fantasy.ToolResponse, error) { + var args activateArgs + if call.Input != "" && call.Input != "{}" { + if err := json.Unmarshal([]byte(call.Input), &args); err != nil { + return fantasy.NewTextErrorResponse(fmt.Sprintf("invalid arguments: %v", err)), nil + } + } + name := strings.TrimSpace(args.Name) + if name == "" { + return fantasy.NewTextErrorResponse("name is required"), nil + } + + // Deduplicate: a skill already activated this session need not be + // re-injected (agentskills.io spec, gap #14). + t.mu.Lock() + already := t.activated[name] + t.mu.Unlock() + if already { + return fantasy.NewTextResponse( + fmt.Sprintf("Skill %q was already loaded earlier in this session.", name)), nil + } + + // Resolve the skill path from the current provider snapshot. + var path string + for _, s := range t.provider() { + if s.Name == name { + path = s.Path + break + } + } + if path == "" { + return fantasy.NewTextErrorResponse(fmt.Sprintf("unknown skill %q", name)), nil + } + + // Re-read the file for freshness, stripping frontmatter. + loaded, err := skills.LoadSkill(path) + if err != nil { + return fantasy.NewTextErrorResponse(fmt.Sprintf("failed to load skill %q: %v", name, err)), nil + } + + var buf strings.Builder + fmt.Fprintf(&buf, "\n", loaded.Name, loaded.Path) + fmt.Fprintf(&buf, "References are relative to %s.\n\n", loaded.BaseDir()) + buf.WriteString(loaded.Content) + if res := skills.FormatResources(loaded.Resources()); res != "" { + buf.WriteString("\n\n") + buf.WriteString(res) + } + buf.WriteString("\n") + + t.mu.Lock() + t.activated[name] = true + t.mu.Unlock() + + return fantasy.NewTextResponse(buf.String()), nil +} diff --git a/internal/skilltool/skilltool_test.go b/internal/skilltool/skilltool_test.go new file mode 100644 index 00000000..97c44470 --- /dev/null +++ b/internal/skilltool/skilltool_test.go @@ -0,0 +1,89 @@ +package skilltool + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "charm.land/fantasy" + + "github.com/mark3labs/kit/internal/skills" +) + +func writeSkillFile(t *testing.T, dir, name string) *skills.Skill { + t.Helper() + skillDir := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Join(skillDir, "scripts"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "scripts", "run.py"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + path := filepath.Join(skillDir, "SKILL.md") + content := "---\nname: " + name + "\ndescription: A test skill\n---\nDo the thing." + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + return &skills.Skill{Name: name, Description: "A test skill", Path: path} +} + +func TestNew_NilWhenNoSkills(t *testing.T) { + if tool := New(nil, func() []*skills.Skill { return nil }); tool != nil { + t.Error("expected nil tool when no skill names provided") + } +} + +func TestActivateSkill_LoadsAndDedups(t *testing.T) { + dir := t.TempDir() + s := writeSkillFile(t, dir, "extract") + provider := func() []*skills.Skill { return []*skills.Skill{s} } + + tool := New([]string{"extract"}, provider) + if tool == nil { + t.Fatal("expected a tool") + } + + // First activation loads content + resources. + resp, err := tool.Run(context.Background(), fantasy.ToolCall{Input: `{"name":"extract"}`}) + if err != nil { + t.Fatal(err) + } + out := responseText(resp) + if !strings.Contains(out, "Do the thing.") { + t.Errorf("expected skill body, got: %q", out) + } + if !strings.Contains(out, "/.agents/skills/ and +// /.kit/skills/) are injected into the system prompt. A freshly +// cloned, untrusted repository could therefore smuggle instructions into the +// agent the moment the user runs Kit inside it. To mitigate this prompt- +// injection vector, project-level skill loading can be gated on an explicit +// trust decision recorded here. +// +// The allowlist is stored as JSON at $XDG_CONFIG_HOME/kit/trusted-projects.json +// (default ~/.config/kit/trusted-projects.json). +package trust + +import ( + "encoding/json" + "os" + "path/filepath" + "sync" +) + +// Decision is the outcome of a trust prompt. +type Decision int + +const ( + // Skip declines to load project skills this session and records nothing. + Skip Decision = iota + // Trust loads project skills this session and persists the directory. + Trust + // TrustOnce loads project skills this session without persisting. + TrustOnce +) + +// storeFileName is the basename of the persisted allowlist. +const storeFileName = "trusted-projects.json" + +// Store is a persisted set of trusted project directories. The zero value is +// not usable — construct one with Load. +type Store struct { + mu sync.Mutex + path string + trusted map[string]bool +} + +// store mirrors the on-disk JSON layout. +type store struct { + Projects []string `json:"projects"` +} + +// DefaultPath returns the path the trust allowlist is persisted to, respecting +// $XDG_CONFIG_HOME. Returns the empty string when no home directory can be +// determined. +func DefaultPath() string { + base := os.Getenv("XDG_CONFIG_HOME") + if base == "" { + home, err := os.UserHomeDir() + if err != nil { + return "" + } + base = filepath.Join(home, ".config") + } + return filepath.Join(base, "kit", storeFileName) +} + +// Load reads the trust allowlist from path. A missing file yields an empty +// store (not an error). Pass an empty path to use DefaultPath. +func Load(path string) (*Store, error) { + if path == "" { + path = DefaultPath() + } + s := &Store{path: path, trusted: map[string]bool{}} + if path == "" { + return s, nil + } + + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return s, nil + } + return s, err + } + + var raw store + if err := json.Unmarshal(data, &raw); err != nil { + return s, err + } + for _, p := range raw.Projects { + s.trusted[normalize(p)] = true + } + return s, nil +} + +// IsTrusted reports whether dir has been marked trusted. +func (s *Store) IsTrusted(dir string) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.trusted[normalize(dir)] +} + +// Trust records dir as trusted and persists the allowlist to disk. +func (s *Store) Trust(dir string) error { + s.mu.Lock() + s.trusted[normalize(dir)] = true + s.mu.Unlock() + return s.save() +} + +// Untrust removes dir from the allowlist and persists the change. +func (s *Store) Untrust(dir string) error { + s.mu.Lock() + delete(s.trusted, normalize(dir)) + s.mu.Unlock() + return s.save() +} + +// save writes the allowlist to disk, creating parent directories as needed. +func (s *Store) save() error { + if s.path == "" { + return nil + } + s.mu.Lock() + projects := make([]string, 0, len(s.trusted)) + for p := range s.trusted { + projects = append(projects, p) + } + s.mu.Unlock() + + data, err := json.MarshalIndent(store{Projects: projects}, "", " ") + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(s.path), 0o755); err != nil { + return err + } + return os.WriteFile(s.path, data, 0o644) +} + +// normalize resolves dir to an absolute, symlink-evaluated path for stable +// comparison. It falls back to the cleaned input when resolution fails. +func normalize(dir string) string { + if dir == "" { + return "" + } + abs, err := filepath.Abs(dir) + if err != nil { + return filepath.Clean(dir) + } + if resolved, err := filepath.EvalSymlinks(abs); err == nil { + return resolved + } + return abs +} diff --git a/internal/trust/trust_test.go b/internal/trust/trust_test.go new file mode 100644 index 00000000..29b7e99a --- /dev/null +++ b/internal/trust/trust_test.go @@ -0,0 +1,60 @@ +package trust + +import ( + "path/filepath" + "testing" +) + +func TestStore_TrustAndPersist(t *testing.T) { + dir := t.TempDir() + storePath := filepath.Join(dir, "trusted-projects.json") + project := filepath.Join(dir, "repo") + + s, err := Load(storePath) + if err != nil { + t.Fatal(err) + } + if s.IsTrusted(project) { + t.Fatal("project should not be trusted initially") + } + if err := s.Trust(project); err != nil { + t.Fatal(err) + } + if !s.IsTrusted(project) { + t.Fatal("project should be trusted after Trust") + } + + // Reload from disk to confirm persistence. + s2, err := Load(storePath) + if err != nil { + t.Fatal(err) + } + if !s2.IsTrusted(project) { + t.Fatal("trust decision should persist across reloads") + } +} + +func TestStore_Untrust(t *testing.T) { + dir := t.TempDir() + storePath := filepath.Join(dir, "trusted-projects.json") + project := filepath.Join(dir, "repo") + + s, _ := Load(storePath) + _ = s.Trust(project) + if err := s.Untrust(project); err != nil { + t.Fatal(err) + } + if s.IsTrusted(project) { + t.Fatal("project should not be trusted after Untrust") + } +} + +func TestStore_MissingFileIsEmpty(t *testing.T) { + s, err := Load(filepath.Join(t.TempDir(), "does-not-exist.json")) + if err != nil { + t.Fatalf("missing file should not error: %v", err) + } + if s.IsTrusted("/anything") { + t.Fatal("empty store should trust nothing") + } +} diff --git a/pkg/kit/kit.go b/pkg/kit/kit.go index 42eb99a3..6f61d643 100644 --- a/pkg/kit/kit.go +++ b/pkg/kit/kit.go @@ -23,6 +23,7 @@ import ( "github.com/mark3labs/kit/internal/models" "github.com/mark3labs/kit/internal/session" "github.com/mark3labs/kit/internal/skills" + "github.com/mark3labs/kit/internal/skilltool" "github.com/mark3labs/kit/internal/tools" "github.com/spf13/viper" @@ -1009,9 +1010,24 @@ type Options struct { // Skills Skills []string // Explicit skill files/dirs to load (empty = auto-discover) - SkillsDir string // Override default project-local skills directory + SkillsDir string // Direct skills directory to scan (overrides auto-discovery; scanned as-is) NoSkills bool // Disable skill loading entirely (auto-discovery and explicit) + // SkillsDisable names skills (by Name) to exclude from the model-facing + // catalog. Disabled skills remain available via the /skill: slash command. + SkillsDisable []string + + // SkillTrustPrompt is an optional callback invoked the first time Kit + // auto-discovers project-local skills (under /.agents/skills or + // /.kit/skills) in a directory that is not yet on the trust + // allowlist. It receives the project directory and the number of skills + // found, and returns a TrustDecision controlling whether the skills load. + // + // When nil, project-local skills are loaded without prompting (historical + // behaviour). Directories trusted via TrustProject are persisted to + // ~/.config/kit/trusted-projects.json and not prompted again. + SkillTrustPrompt func(projectDir string, skillCount int) TrustDecision + // NoExtensions disables Yaegi extension loading entirely. NoExtensions bool @@ -1373,6 +1389,15 @@ func New(ctx context.Context, opts *Options) (*Kit, error) { if err != nil { return fmt.Errorf("failed to load skills: %w", err) } + + // Apply per-skill disable list (--skill-disable / skill-disable + // config key). Disabled skills stay loaded (so /skill: still + // works) but are hidden from the model-facing catalog. + disable := opts.SkillsDisable + if len(disable) == 0 { + disable = v.GetStringSlice("skill-disable") + } + applySkillDisableList(loadedSkills, disable) } // Always compose the system prompt with runtime context: base prompt + @@ -1526,12 +1551,37 @@ func New(ctx context.Context, opts *Options) (*Kit, error) { // Build agent setup options, pulling CLI-specific fields when available. // Pass the pre-built ProviderConfig and scalar viper snapshots so // SetupAgent doesn't need to re-read viper (which would require the lock). + + // Register the dedicated activate_skill tool when at least one skill is + // loaded (issue #65, gaps #13/#14). The provider closure reads the live + // skill set from the Kit instance once it exists so runtime additions + // resolve; skillToolKit is assigned after construction below. + var skillToolKit *Kit + extraTools := opts.ExtraTools + if len(loadedSkills) > 0 { + names := make([]string, 0, len(loadedSkills)) + for _, s := range loadedSkills { + if !s.DisableModelInvocation { + names = append(names, s.Name) + } + } + provider := func() []*skills.Skill { + if skillToolKit == nil { + return loadedSkills + } + return skillToolKit.GetSkills() + } + if t := skilltool.New(names, provider); t != nil { + extraTools = append(extraTools, t) + } + } + setupOpts := kitsetup.AgentSetupOptions{ MCPConfig: mcpConfig, Quiet: opts.Quiet, CoreTools: opts.Tools, DisableCoreTools: disableCoreTools, - ExtraTools: opts.ExtraTools, + ExtraTools: extraTools, ToolWrapper: hookToolWrapper(beforeToolCall, afterToolResult), ProviderConfig: providerConfig, Debug: debug, @@ -1631,6 +1681,10 @@ func New(ctx context.Context, opts *Options) (*Kit, error) { prepareStep: prepareStep, } + // Point the activate_skill provider closure at the live Kit instance so it + // resolves skills mutated after construction. + skillToolKit = k + // Bridge extension events to SDK hooks. if agentResult.ExtRunner != nil { k.bridgeExtensions(agentResult.ExtRunner) @@ -1741,6 +1795,14 @@ func (m *Kit) expandSkillCommand(prompt string) string { fmt.Fprintf(&buf, "\n", loaded.Name, loaded.Path) fmt.Fprintf(&buf, "References are relative to %s.\n\n", baseDir) buf.WriteString(loaded.Content) + + // Enumerate bundled resources (scripts/, references/, assets/) so the model + // knows what it can read without listing the directory itself. + if res := skills.FormatResources(loaded.Resources()); res != "" { + buf.WriteString("\n\n") + buf.WriteString(res) + } + buf.WriteString("\n") args = strings.TrimSpace(args) @@ -1757,18 +1819,33 @@ func (m *Kit) expandSkillCommand(prompt string) string { // --------------------------------------------------------------------------- // loadSkills loads skills based on Options. If explicit paths are provided -// they are loaded directly; otherwise auto-discovery runs. +// they are loaded directly. If SkillsDir is set it is treated as a direct +// skills directory (scanned as-is, not as a parent of .agents/.kit). Otherwise +// auto-discovery runs against the standard scopes rooted at SessionDir. func loadSkills(opts *Options) ([]*skills.Skill, error) { if len(opts.Skills) > 0 { return loadExplicitSkills(opts.Skills) } - // Auto-discover from standard directories. - cwd := opts.SkillsDir + // An explicit --skills-dir is a direct skills directory: scan it as-is + // rather than appending .agents/skills and .kit/skills beneath it. + if opts.SkillsDir != "" { + return skills.LoadSkillsFromDir(opts.SkillsDir) + } + + // Auto-discover from the standard scopes rooted at the session directory. + // Project-local skills are injected into the system prompt, so they are + // gated on a trust decision when a SkillTrustPrompt is configured. + cwd := opts.SessionDir if cwd == "" { - cwd = opts.SessionDir + cwd, _ = os.Getwd() + } + user := skills.LoadUserSkills() + project := skills.LoadProjectSkills(cwd) + if len(project) > 0 && !projectSkillsTrusted(opts, cwd, len(project)) { + project = nil } - return skills.LoadSkills(cwd) + return skills.Combine(user, project), nil } // loadExplicitSkills loads skills from a list of explicit paths. Each path diff --git a/pkg/kit/skills.go b/pkg/kit/skills.go index 24aeea3c..9369b243 100644 --- a/pkg/kit/skills.go +++ b/pkg/kit/skills.go @@ -7,6 +7,7 @@ import ( "github.com/mark3labs/kit/internal/extensions" "github.com/mark3labs/kit/internal/skills" + "github.com/mark3labs/kit/internal/trust" ) // ==== Skills Types ==== @@ -51,11 +52,14 @@ func LoadSkillsFromFS(fsys fs.FS, root string) ([]*Skill, error) { } // LoadSkills auto-discovers skills from standard directories: -// - Global: $XDG_CONFIG_HOME/kit/skills/ (default ~/.config/kit/skills/) -// - Project-local: /.kit/skills/ +// - User-level: ~/.agents/skills/ (cross-client convention) +// - User-level: $XDG_CONFIG_HOME/kit/skills/ (default ~/.config/kit/skills/) +// - Project-local: /.agents/skills/ (cross-client convention) +// - Project-local: /.kit/skills/ (Kit-specific) // -// cwd is the working directory for project-local discovery; if empty the -// current working directory is used. +// Project-level skills take precedence over user-level skills with the same +// name. cwd is the working directory for project-local discovery; if empty +// the current working directory is used. func LoadSkills(cwd string) ([]*Skill, error) { return skills.LoadSkills(cwd) } @@ -127,12 +131,17 @@ func (m *Kit) LoadSkillsFromDirForExtension(dir string) extensions.SkillLoadResu // convertSkill converts internal skill to extension-facing format. func (m *Kit) convertSkill(s *skills.Skill) *extensions.Skill { return &extensions.Skill{ - Name: s.Name, - Description: s.Description, - Content: s.Content, - Path: s.Path, - Tags: s.Tags, - When: s.When, + Name: s.Name, + Description: s.Description, + Content: s.Content, + Path: s.Path, + License: s.License, + Compatibility: s.Compatibility, + Metadata: s.Metadata, + AllowedTools: s.AllowedTools, + DisableModelInvocation: s.DisableModelInvocation, + Tags: s.Tags, + When: s.When, } } @@ -300,3 +309,107 @@ func (m *Kit) applyComposedSystemPrompt() { func (m *Kit) RefreshSystemPrompt() { m.applyComposedSystemPrompt() } + +// --------------------------------------------------------------------------- +// Per-skill disable (Issue #65, gap #10) +// --------------------------------------------------------------------------- + +// applySkillDisableList sets DisableModelInvocation on every skill whose Name +// appears in names. Disabled skills remain loaded (so explicit /skill: +// activation still works) but are hidden from the model-facing catalog. +func applySkillDisableList(skillList []*skills.Skill, names []string) { + if len(names) == 0 { + return + } + disabled := make(map[string]bool, len(names)) + for _, n := range names { + disabled[n] = true + } + for _, s := range skillList { + if disabled[s.Name] { + s.DisableModelInvocation = true + } + } +} + +// DisableSkill hides the named skill from the model-facing catalog while +// keeping it loaded (so it can still be activated explicitly via /skill:). +// The system prompt is recomposed and applied. Returns true when a skill with +// that name was found. +func (m *Kit) DisableSkill(name string) bool { + return m.setSkillModelInvocation(name, true) +} + +// EnableSkill re-exposes a previously disabled skill in the model-facing +// catalog. The system prompt is recomposed and applied. Returns true when a +// skill with that name was found. +func (m *Kit) EnableSkill(name string) bool { + return m.setSkillModelInvocation(name, false) +} + +// setSkillModelInvocation toggles DisableModelInvocation on the named skill +// and refreshes the system prompt. Returns true when the skill was found. +func (m *Kit) setSkillModelInvocation(name string, disabled bool) bool { + m.runtimeMu.Lock() + found := false + for _, s := range m.skills { + if s.Name == name { + s.DisableModelInvocation = disabled + found = true + break + } + } + m.runtimeMu.Unlock() + + if !found { + return false + } + m.ClearSkillCache() + m.applyComposedSystemPrompt() + return true +} + +// --------------------------------------------------------------------------- +// Project-skill trust gate (Issue #65, gap #8) +// --------------------------------------------------------------------------- + +// TrustDecision is the outcome of a project-skill trust prompt. +type TrustDecision = trust.Decision + +// Trust-prompt outcomes. They mirror the trust package decisions. +const ( + // SkipProjectSkills declines to load project skills this session. + SkipProjectSkills = trust.Skip + // TrustProject loads project skills and persists the directory as trusted. + TrustProject = trust.Trust + // TrustProjectOnce loads project skills this session without persisting. + TrustProjectOnce = trust.TrustOnce +) + +// projectSkillsTrusted decides whether project-local skills discovered in dir +// should be loaded. When no SkillTrustPrompt is configured the directory is +// trusted by default (preserving historical behaviour). Otherwise a persisted +// allowlist is consulted first, then the prompt is invoked for an unknown +// directory and the decision is persisted when the user chooses TrustProject. +func projectSkillsTrusted(opts *Options, dir string, count int) bool { + if opts.SkillTrustPrompt == nil { + return true + } + + store, err := trust.Load("") + if err == nil && store.IsTrusted(dir) { + return true + } + + switch opts.SkillTrustPrompt(dir, count) { + case TrustProject: + if store != nil { + _ = store.Trust(dir) + } + return true + case TrustProjectOnce: + return true + default: + return false + } +} diff --git a/pkg/kit/skills_spec_test.go b/pkg/kit/skills_spec_test.go new file mode 100644 index 00000000..37d2e639 --- /dev/null +++ b/pkg/kit/skills_spec_test.go @@ -0,0 +1,120 @@ +package kit + +import ( + "os" + "path/filepath" + "testing" +) + +func writeSkillFile(t *testing.T, path, name, desc string) { + t.Helper() + content := "---\nname: " + name + "\ndescription: " + desc + "\n---\nBody." + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// TestLoadSkills_SkillsDirIsDirect verifies --skills-dir scans the directory +// directly rather than appending .agents/.kit beneath it (issue #65, gap #3). +func TestLoadSkills_SkillsDirIsDirect(t *testing.T) { + dir := t.TempDir() + writeSkillFile(t, filepath.Join(dir, "direct.md"), "direct", "A direct skill") + + got, err := loadSkills(&Options{SkillsDir: dir}) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || got[0].Name != "direct" { + t.Fatalf("expected 1 skill named 'direct', got %+v", got) + } +} + +// TestApplySkillDisableList verifies the disable list hides a skill from the +// catalog (issue #65, gap #10). +func TestApplySkillDisableList(t *testing.T) { + dir := t.TempDir() + writeSkillFile(t, filepath.Join(dir, "a.md"), "a", "skill a") + writeSkillFile(t, filepath.Join(dir, "b.md"), "b", "skill b") + + got, err := loadSkills(&Options{SkillsDir: dir}) + if err != nil { + t.Fatal(err) + } + applySkillDisableList(got, []string{"a"}) + + var aDisabled, bDisabled bool + for _, s := range got { + switch s.Name { + case "a": + aDisabled = s.DisableModelInvocation + case "b": + bDisabled = s.DisableModelInvocation + } + } + if !aDisabled { + t.Error("skill 'a' should be disabled") + } + if bDisabled { + t.Error("skill 'b' should not be disabled") + } +} + +// TestProjectSkillsTrust verifies the trust gate drops untrusted project +// skills and honours a Trust decision (issue #65, gap #8). +func TestProjectSkillsTrust(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", filepath.Join(dir, "xdg")) + t.Setenv("HOME", filepath.Join(dir, "home")) + + projectDir := filepath.Join(dir, "repo") + skillsDir := filepath.Join(projectDir, ".agents", "skills") + if err := os.MkdirAll(skillsDir, 0o755); err != nil { + t.Fatal(err) + } + writeSkillFile(t, filepath.Join(skillsDir, "proj.md"), "proj", "project skill") + + // Skip decision → no project skills loaded. + skipped, err := loadSkills(&Options{ + SessionDir: projectDir, + SkillTrustPrompt: func(_ string, _ int) TrustDecision { + return SkipProjectSkills + }, + }) + if err != nil { + t.Fatal(err) + } + if len(skipped) != 0 { + t.Fatalf("expected 0 skills when trust is skipped, got %d", len(skipped)) + } + + // Trust decision → project skills loaded and directory persisted. + prompted := 0 + trusted, err := loadSkills(&Options{ + SessionDir: projectDir, + SkillTrustPrompt: func(_ string, _ int) TrustDecision { + prompted++ + return TrustProject + }, + }) + if err != nil { + t.Fatal(err) + } + if len(trusted) != 1 { + t.Fatalf("expected 1 skill when trusted, got %d", len(trusted)) + } + + // A subsequent load should not prompt again (persisted trust). + again, err := loadSkills(&Options{ + SessionDir: projectDir, + SkillTrustPrompt: func(_ string, _ int) TrustDecision { + t.Error("should not prompt for an already-trusted directory") + return SkipProjectSkills + }, + }) + if err != nil { + t.Fatal(err) + } + if len(again) != 1 { + t.Fatalf("expected 1 skill on trusted reload, got %d", len(again)) + } +} diff --git a/www/pages/cli/commands.md b/www/pages/cli/commands.md index aa3b1081..e4a055f8 100644 --- a/www/pages/cli/commands.md +++ b/www/pages/cli/commands.md @@ -67,14 +67,61 @@ kit --skill path/to/skill.md "prompt" # Load multiple skill files or directories (flag is repeatable) kit --skill ./skill1.md --skill ./skill2.md "prompt" -# Load all skills from a custom directory instead of the default locations +# Scan a directory directly for skills (overrides auto-discovery) kit --skills-dir /path/to/skills "prompt" +# Hide a skill from the model catalog by name (still usable via /skill:) +kit --skill-disable noisy-skill "prompt" + # Disable all skill loading (auto-discovery and explicit) kit --no-skills "prompt" ``` -Skills are auto-discovered from `~/.config/kit/skills/`, `.kit/skills/`, and `.agents/skills/` by default. Use `--skills-dir` to override the project-local search root, or `--skill` to load files explicitly (which disables auto-discovery). `--no-skills` suppresses all skill loading regardless of other flags. +Skills follow the [agentskills.io](https://agentskills.io/specification) convention. They are auto-discovered from four canonical scopes: + +| Scope | Location | +|-------|----------| +| User-level (cross-client) | `~/.agents/skills/` | +| User-level (Kit) | `~/.config/kit/skills/` (honors `$XDG_CONFIG_HOME`) | +| Project-local (cross-client) | `/.agents/skills/` | +| Project-local (Kit) | `/.kit/skills/` | + +When two skills share the same `name`, the project-level one takes precedence over the user-level one. Use `--skills-dir` to scan one directory directly instead (it is **not** treated as a parent of `.agents`/`.kit` — the directory itself is scanned). `--skill` loads files explicitly (which disables auto-discovery), and `--no-skills` suppresses all skill loading regardless of other flags. + +Disabled skills (`--skill-disable`, the `skill-disable` config key, or `disable-model-invocation: true` in a skill's frontmatter) are hidden from the model-facing `` catalog but remain available for explicit activation via the `/skill:` command. + +### Skill frontmatter + +A skill is a markdown file (`SKILL.md` in a directory, or a standalone `.md`/`.txt` file) with optional YAML frontmatter. Kit reads the full [agentskills.io](https://agentskills.io/specification) field set plus two Kit-specific extensions: + +```yaml +--- +name: pdf-extractor # required +description: Use when extracting tables from PDFs # required (drives model discovery) +license: MIT # optional, SPDX identifier +compatibility: claude-code, cursor # optional, targeted environments +allowed-tools: read, bash # optional (experimental) tool restriction +disable-model-invocation: false # optional; true hides from the catalog +metadata: # optional arbitrary key/value pairs + author: you +tags: [pdf, data] # Kit extension +when: on-demand # Kit extension +--- +``` + +`name` and `description` are required — a skill missing its description is skipped with a logged warning, since the description is the sole basis on which the model decides relevance. Descriptions are XML-escaped before they enter the catalog, so characters like `<`, `>`, and `&` are safe. A skill directory may bundle `scripts/`, `references/`, and `assets/` subdirectories; when a skill is activated those files are enumerated in a `` block so the model knows what it can read. + +### Project trust prompt + +Because project-local skills are injected into the system prompt, entering a repository that ships `.agents/skills/` or `.kit/skills/` for the first time prompts you to trust it before any project skill loads — a safeguard against a freshly cloned, untrusted repo smuggling instructions into the agent: + +``` +This project provides 2 skills under .agents/skills or .kit/skills: + /path/to/repo +Load them into the agent? [t]rust always / [o]nce / [s]kip (default skip): +``` + +Choosing **trust always** persists the directory to `~/.config/kit/trusted-projects.json` so you are not asked again. The prompt is skipped (skills load silently) in non-interactive runs — when a prompt is passed positionally, `--quiet` is set, or stdin is not a TTY. ## GitHub integration diff --git a/www/pages/cli/flags.md b/www/pages/cli/flags.md index 17f46590..10b79c87 100644 --- a/www/pages/cli/flags.md +++ b/www/pages/cli/flags.md @@ -53,7 +53,8 @@ These flags control Kit's behavior. When a prompt is passed as a positional argu | Flag | Short | Default | Description | |------|-------|---------|-------------| | `--skill` | — | — | Load skill file or directory (repeatable) | -| `--skills-dir` | — | — | Override the project-local skills directory for auto-discovery | +| `--skills-dir` | — | — | Scan this directory directly for skills (overrides auto-discovery) | +| `--skill-disable` | — | — | Hide a skill from the model catalog by name (repeatable); still usable via `/skill:` | | `--no-skills` | — | `false` | Disable skill loading (auto-discovery and explicit) | ## Generation parameters diff --git a/www/pages/configuration.md b/www/pages/configuration.md index 15fe5302..3fd9ffbd 100644 --- a/www/pages/configuration.md +++ b/www/pages/configuration.md @@ -49,7 +49,8 @@ stream: true | `prompt-template` | string | — | Specific template to load by name | | `no-skills` | bool | `false` | Disable skill loading (auto-discovery and explicit) | | `skill` | list | — | Explicit skill files or directories to load (disables auto-discovery) | -| `skills-dir` | string | — | Override the project-local directory used for skill auto-discovery | +| `skills-dir` | string | — | Scan this directory directly for skills (overrides auto-discovery; not treated as a parent of `.agents`/`.kit`) | +| `skill-disable` | list | — | Skill names to hide from the model catalog (still usable via `/skill:`) | ## Environment variables diff --git a/www/pages/extensions/capabilities.md b/www/pages/extensions/capabilities.md index 2eeded4f..04093df0 100644 --- a/www/pages/extensions/capabilities.md +++ b/www/pages/extensions/capabilities.md @@ -447,11 +447,14 @@ Load and inject skills dynamically at runtime: ```go // Discover skills from standard locations result := ctx.DiscoverSkills() // ext.SkillLoadResult{Skills, Error} -// Standard locations: ~/.config/kit/skills/, .kit/skills/, .agents/skills/ +// Standard locations: ~/.agents/skills/, ~/.config/kit/skills/, +// /.agents/skills/, /.kit/skills/ // Load a specific skill file skill, err := ctx.LoadSkill("/path/to/skill.md") // (*ext.Skill, error string) -// skill.Name, skill.Description, skill.Content, skill.Tags, skill.When +// Spec fields: skill.Name, skill.Description, skill.License, skill.Compatibility, +// skill.Metadata, skill.AllowedTools, skill.DisableModelInvocation +// Plus content/path and Kit extensions: skill.Content, skill.Path, skill.Tags, skill.When // Load all skills from a directory result := ctx.LoadSkillsFromDir("/path/to/skills") // ext.SkillLoadResult diff --git a/www/pages/sdk/options.md b/www/pages/sdk/options.md index 931d2b7a..d36e6e67 100644 --- a/www/pages/sdk/options.md +++ b/www/pages/sdk/options.md @@ -65,9 +65,10 @@ host, err := kit.New(ctx, &kit.Options{ AutoCompact: true, // Skills - Skills: []string{"/path/to/skill.md"}, - SkillsDir: "/path/to/skills/", - NoSkills: true, + Skills: []string{"/path/to/skill.md"}, + SkillsDir: "/path/to/skills/", + SkillsDisable: []string{"noisy-skill"}, + NoSkills: true, // Feature toggles NoExtensions: true, // disable Yaegi extension loading @@ -170,13 +171,48 @@ when embedding Kit as a library. |-------|------|---------|-------------| | `SkipConfig` | `bool` | `false` | Skip `.kit.yml` file loading (viper defaults + env vars still apply) | | `Skills` | `[]string` | — | Explicit skill files/dirs to load | -| `SkillsDir` | `string` | — | Override default skills directory | +| `SkillsDir` | `string` | — | Scan this directory directly for skills (overrides auto-discovery; scanned as-is) | +| `SkillsDisable` | `[]string` | — | Skill names to hide from the model catalog (still usable via `/skill:`) | +| `SkillTrustPrompt` | `func(projectDir string, skillCount int) TrustDecision` | `nil` | Callback gating project-local skill loading on a trust decision (see below) | | `NoSkills` | `bool` | `false` | Disable skill loading entirely | +#### Project-skill trust gate + +Project-local skills (under `/.agents/skills/` or `/.kit/skills/`) +are injected into the system prompt, so loading them from an untrusted, freshly +cloned repository is a prompt-injection vector. Set `SkillTrustPrompt` to gate +that first load on an explicit decision. When `nil` (the default), project +skills load without prompting — preserving historical behaviour. + +```go +opts := &kit.Options{ + SkillTrustPrompt: func(projectDir string, skillCount int) kit.TrustDecision { + // Consult your own UI / policy here. + if userApproves(projectDir, skillCount) { + return kit.TrustProject // load and persist the directory as trusted + } + return kit.SkipProjectSkills // do not load project skills + }, +} +``` + +The callback returns one of three `TrustDecision` values: + +| Decision | Effect | +|----------|--------| +| `kit.TrustProject` | Load project skills and persist `projectDir` to `~/.config/kit/trusted-projects.json` (not prompted again) | +| `kit.TrustProjectOnce` | Load project skills for this run only, without persisting | +| `kit.SkipProjectSkills` | Do not load project skills | + +A directory already on the persisted allowlist is trusted without invoking the +callback. The Kit CLI wires this to an interactive terminal prompt automatically +for TTY sessions. + These fields only control the **initial** skill and context-file set picked up by `New()`. To add, remove, or replace skills and `AGENTS.md`-style context files at runtime (e.g. per user or per session), use the -`AddSkill` / `LoadAndAddSkill` / `RemoveSkill` / `SetSkills` and +`AddSkill` / `LoadAndAddSkill` / `RemoveSkill` / `SetSkills` / +`DisableSkill` / `EnableSkill` and `AddContextFile` / `AddContextFileContent` / `RemoveContextFile` / `SetContextFiles` methods on `*kit.Kit`. See [Runtime skills and context files](/sdk/overview#runtime-skills-and-context-files). diff --git a/www/pages/sdk/overview.md b/www/pages/sdk/overview.md index 04f83698..5def1580 100644 --- a/www/pages/sdk/overview.md +++ b/www/pages/sdk/overview.md @@ -382,6 +382,11 @@ host.LoadAndAddContextFile("/etc/agents/tenant-acme.md") host.RemoveSkill("polite-french") host.RemoveContextFile(fmt.Sprintf("session://%s/AGENTS.md", userID)) +// Hide a skill from the model-facing catalog without unloading it — it stays +// available for explicit /skill: activation. EnableSkill reverses this. +host.DisableSkill("refund-policy") +host.EnableSkill("refund-policy") + // Or replace the whole set in one call. host.SetSkills(activeSkillsForUser) host.SetContextFiles(activeContextForUser) @@ -407,9 +412,22 @@ Key points: from multiple goroutines; the underlying state is guarded by an internal `RWMutex`. - **Init-time options still apply.** `Options.Skills`, `Options.SkillsDir`, - `Options.NoSkills`, and `Options.NoContextFiles` continue to control the - startup set; the runtime API mutates from whatever state `New()` produced. + `Options.SkillsDisable`, `Options.SkillTrustPrompt`, `Options.NoSkills`, and + `Options.NoContextFiles` continue to control the startup set; the runtime API + mutates from whatever state `New()` produced. See [SDK options](/sdk/options#skills--configuration). +- **Auto-discovery scopes.** When no explicit `Skills`/`SkillsDir` are given, + `New()` scans four [agentskills.io](https://agentskills.io/specification) + locations: `~/.agents/skills/`, `~/.config/kit/skills/`, + `/.agents/skills/`, and `/.kit/skills/`. Project-level + skills override user-level skills of the same `name`. Skills missing a + `description` are skipped with a logged warning, and a skill's + `disable-model-invocation: true` (or `Options.SkillsDisable`) hides it from + the catalog while keeping it available for explicit activation. +- **Skill helpers.** A `kit.Skill` exposes `BaseDir()` (its directory) and + `Resources()` (the files bundled under `scripts/`, `references/`, and + `assets/`), which power the `` enumeration shown when a skill + is activated. - **`fs.FS`-backed discovery.** The package-level loaders `kit.LoadSkill`, `kit.LoadSkillsFromDir`, and `kit.LoadSkills` are path-string based; `kit.LoadSkillsFromFS(fsys, root)` is the `fs.FS`-typed counterpart for From 902ad8a30e8f333dd8c1535c39bb92f28ee4ad88 Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Thu, 18 Jun 2026 19:30:40 +0300 Subject: [PATCH 2/2] fix(skills): address skill loading and activation review findings - log (instead of discard) genuine errors from skill directory loads so permission/read failures no longer yield a silently partial catalog - make activate_skill dedup atomic by holding the lock across check and mark, preventing concurrent double-activation - reject activation of disable-model-invocation skills in the tool's runtime lookup, mirroring their catalog/enum exclusion - add regression test for disabled-skill activation --- internal/skills/skills.go | 20 +++++++++++++++++--- internal/skilltool/skilltool.go | 21 ++++++++++++--------- internal/skilltool/skilltool_test.go | 18 ++++++++++++++++++ 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 1d00275a..78236ee7 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -414,11 +414,22 @@ func LoadSkillsFromFS(fsys fs.FS, root string) ([]*Skill, error) { func LoadUserSkills() []*Skill { var loaded []*Skill if home, err := os.UserHomeDir(); err == nil && home != "" { - ss, _ := LoadSkillsFromDir(filepath.Join(home, ".agents", "skills")) + dir := filepath.Join(home, ".agents", "skills") + ss, loadErr := LoadSkillsFromDir(dir) + if loadErr != nil { + // Missing directories are already swallowed by LoadSkillsFromDir, + // so a non-nil error here is genuine (permission denied, read + // failure, or a malformed skill file) and would otherwise yield a + // silently partial catalog. + log.Warn("failed to load some user skills", "dir", dir, "err", loadErr) + } loaded = append(loaded, ss...) } if g := globalSkillsDir(); g != "" { - ss, _ := LoadSkillsFromDir(g) + ss, loadErr := LoadSkillsFromDir(g) + if loadErr != nil { + log.Warn("failed to load some user skills", "dir", g, "err", loadErr) + } loaded = append(loaded, ss...) } for _, s := range loaded { @@ -445,7 +456,10 @@ func LoadProjectSkills(cwd string) []*Skill { filepath.Join(cwd, ".agents", "skills"), filepath.Join(cwd, ".kit", "skills"), } { - ss, _ := LoadSkillsFromDir(dir) + ss, loadErr := LoadSkillsFromDir(dir) + if loadErr != nil { + log.Warn("failed to load some project skills", "dir", dir, "err", loadErr) + } loaded = append(loaded, ss...) } for _, s := range loaded { diff --git a/internal/skilltool/skilltool.go b/internal/skilltool/skilltool.go index 08124172..ce3b36a4 100644 --- a/internal/skilltool/skilltool.go +++ b/internal/skilltool/skilltool.go @@ -93,20 +93,25 @@ func (t *activateSkillTool) Run(_ context.Context, call fantasy.ToolCall) (fanta return fantasy.NewTextErrorResponse("name is required"), nil } - // Deduplicate: a skill already activated this session need not be - // re-injected (agentskills.io spec, gap #14). + // Hold the lock across the whole activation so the dedup check and the + // subsequent mark are atomic — two concurrent calls cannot both pass the + // check and double-activate the same skill (gap #14). The skill is only + // marked activated on success, so a failed load can be retried. t.mu.Lock() - already := t.activated[name] - t.mu.Unlock() - if already { + defer t.mu.Unlock() + + if t.activated[name] { return fantasy.NewTextResponse( fmt.Sprintf("Skill %q was already loaded earlier in this session.", name)), nil } - // Resolve the skill path from the current provider snapshot. + // Resolve the skill path from the current provider snapshot. Skills with + // disable-model-invocation set are not activatable by the model (they + // remain available via the /skill: command), mirroring their exclusion + // from the catalog and the tool's name enum. var path string for _, s := range t.provider() { - if s.Name == name { + if s.Name == name && !s.DisableModelInvocation { path = s.Path break } @@ -131,9 +136,7 @@ func (t *activateSkillTool) Run(_ context.Context, call fantasy.ToolCall) (fanta } buf.WriteString("\n") - t.mu.Lock() t.activated[name] = true - t.mu.Unlock() return fantasy.NewTextResponse(buf.String()), nil } diff --git a/internal/skilltool/skilltool_test.go b/internal/skilltool/skilltool_test.go index 97c44470..dcbf0456 100644 --- a/internal/skilltool/skilltool_test.go +++ b/internal/skilltool/skilltool_test.go @@ -83,6 +83,24 @@ func TestActivateSkill_UnknownSkill(t *testing.T) { } } +// TestActivateSkill_DisabledNotActivatable verifies a skill flagged +// disable-model-invocation cannot be activated through the model-facing tool. +func TestActivateSkill_DisabledNotActivatable(t *testing.T) { + dir := t.TempDir() + s := writeSkillFile(t, dir, "extract") + s.DisableModelInvocation = true + provider := func() []*skills.Skill { return []*skills.Skill{s} } + + tool := New([]string{"extract"}, provider) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{Input: `{"name":"extract"}`}) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(responseText(resp), "unknown skill") { + t.Errorf("disabled skill should not be activatable, got: %q", responseText(resp)) + } +} + // responseText extracts the text from a tool response. func responseText(resp fantasy.ToolResponse) string { return resp.Content