From b63ff274cd3a40fb2e01ecca359180ab4d88fa5f Mon Sep 17 00:00:00 2001 From: MK Date: Mon, 6 Jul 2026 22:28:06 -0400 Subject: [PATCH 1/2] fix(skills): align skill catalog names with read_skill resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The system-prompt skill catalog advertised each skill by its internal '## Tool:' heading name (e.g. 'k8s_triage'), but read_skill resolves a skill by its loadable name — the frontmatter 'name' / directory (e.g. 'k8s-incident-triage', which is also what the agent card advertises). When the two differed, the LLM called read_skill with the advertised tool name and got 'skill not found', then abandoned the skill. Changes: - buildSkillCatalog now emits one line per skill keyed by the loadable name (frontmatter name, falling back to directory/file) — the exact string read_skill resolves and the agent card advertises. Each line also carries a 'provides: ' list so the LLM still sees the skill's capabilities for tool selection, without confusing them for the read_skill argument. Script/binary-backed '## Tool:' entries are excluded from 'provides' because they are registered as first-class callable tools (registerSkillTools) and invoked directly by name. - read_skill resolves via a frontmatter-name index (keyed by both the frontmatter name and the directory/file name, normalized for case and underscore/hyphen drift) in addition to the direct path lookup; a miss now returns the available skill names instead of a bare error. Regression tests: read_skill resolves by frontmatter name when the directory differs, normalized variants, flat layout, not-found lists available skills; catalog advertises the loadable name (not the tool heading) while surfacing tool names under 'provides'; and a catalog<->read_skill contract test asserting every advertised name resolves (the gap that let this ship). --- forge-cli/runtime/runner.go | 133 +++++++++++++----- .../runtime/runner_skill_catalog_test.go | 94 +++++++++++++ forge-core/tools/builtins/read_skill.go | 133 ++++++++++++++---- forge-core/tools/builtins/read_skill_test.go | 106 ++++++++++++++ 4 files changed, 407 insertions(+), 59 deletions(-) create mode 100644 forge-cli/runtime/runner_skill_catalog_test.go create mode 100644 forge-core/tools/builtins/read_skill_test.go diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index cded21f..a470d6c 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -3122,6 +3122,23 @@ func (r *Runner) buildSystemPrompt() string { // buildSkillCatalog generates a lightweight catalog of binary-backed skills // (those without scripts) for the system prompt. Script-backed skills are // already registered as first-class tools and don't need catalog entries. +// skillEntryHasScript reports whether a `## Tool:` entry is backed by a +// script on disk, mirroring registerSkillTools' lookup. A script-backed +// entry is registered as a first-class callable tool, so it is excluded +// from the read_skill catalog's "provides" list. +func (r *Runner) skillEntryHasScript(skillDir, toolName string) bool { + scriptName := strings.ReplaceAll(toolName, "_", "-") + if skillDir != "" { + if _, err := os.Stat(filepath.Join(r.cfg.WorkDir, "skills", skillDir, "scripts", scriptName+".sh")); err == nil { + return true + } + } + if _, err := os.Stat(filepath.Join(r.cfg.WorkDir, "skills", "scripts", scriptName+".sh")); err == nil { + return true + } + return false +} + func (r *Runner) buildSkillCatalog() string { matches := r.discoverSkillFiles() if len(matches) == 0 { @@ -3141,50 +3158,95 @@ func (r *Runner) buildSkillCatalog() string { catalogSkillDir = filepath.Base(filepath.Dir(match)) } - // If no ## Tool: entries were parsed but frontmatter has name+description, - // create a synthetic entry so the skill appears in the catalog summary. - if len(entries) == 0 && meta != nil && meta.Name != "" && meta.Description != "" { - entries = []contract.SkillEntry{{ - Name: meta.Name, - Description: meta.Description, - Metadata: meta, - }} + // The identifier advertised here MUST be the one the LLM passes + // to read_skill, which resolves a skill by its loadable name: + // the frontmatter `name` (also what the agent card advertises), + // falling back to the skill directory / flat-file name. Listing a + // `## Tool:` heading name here instead was the skill-lookup bug — + // read_skill("") 404'd whenever the tool name differed from + // the skill's name/directory (e.g. tool "k8s_triage" in skill + // "k8s-incident-triage"). One catalog line per skill, not per tool. + loadName := "" + switch { + case meta != nil && meta.Name != "": + loadName = meta.Name + case catalogSkillDir != "": + loadName = catalogSkillDir + default: + loadName = strings.TrimSuffix(filepath.Base(match), ".md") } - for _, entry := range entries { - // Skip skills that have scripts (already registered as tools) - scriptName := strings.ReplaceAll(entry.Name, "_", "-") - hasScript := false - // Check subdirectory layout: skills/{dir}/scripts/{name}.sh - if catalogSkillDir != "" { - sp := filepath.Join(r.cfg.WorkDir, "skills", catalogSkillDir, "scripts", scriptName+".sh") - if _, err := os.Stat(sp); err == nil { - hasScript = true + // Description: prefer the frontmatter summary, else the first + // parsed tool entry that carries one. + desc := "" + if meta != nil { + desc = meta.Description + } + if desc == "" { + for _, e := range entries { + if e.Description != "" { + desc = e.Description + break } } - // Check legacy flat layout: skills/scripts/{name}.sh - if !hasScript { - sp := filepath.Join(r.cfg.WorkDir, "skills", "scripts", scriptName+".sh") - if _, err := os.Stat(sp); err == nil { - hasScript = true + } + + if loadName == "" || desc == "" { + continue + } + + // A `## Tool:` entry that is script- or binary-backed is registered + // as a first-class callable tool (see registerSkillTools) — the LLM + // invokes it directly by name and it must NOT appear here. Everything + // else is an instruction-only capability that the LLM reaches by + // loading this skill and following its steps. Collect those so the + // catalog surfaces WHAT the skill can do (tool selection) while the + // line's leading identifier stays the read_skill key (skill loading). + runtimeMode := contract.SkillRuntimeScript + if meta != nil && meta.Metadata != nil { + if forgeMap, ok := meta.Metadata["forge"]; ok { + if raw, ok := forgeMap["runtime"]; ok { + if s, ok := raw.(string); ok && s != "" { + runtimeMode = s + } } } - if hasScript { + } + var provides []string + usesCLI := false + for _, e := range entries { + if e.Name == "" { continue } - - if entry.Name != "" && entry.Description != "" { - line := fmt.Sprintf("- %s: %s", entry.Name, entry.Description) - // Note that skill uses cli_execute without listing specific - // binary names — the LLM already sees the allowed enum in the - // tool schema, and listing names here leaks internal tooling - // when users ask "what skills/tools do you have?" - if entry.ForgeReqs != nil && len(entry.ForgeReqs.Bins) > 0 { - line += " (uses cli_execute)" - } - catalogEntries = append(catalogEntries, line) + if e.ForgeReqs != nil && len(e.ForgeReqs.Bins) > 0 { + usesCLI = true + } + if runtimeMode == contract.SkillRuntimeBinary || r.skillEntryHasScript(catalogSkillDir, e.Name) { + continue // registered as a callable tool; not a read_skill capability } + provides = append(provides, e.Name) + } + + // A fully script/binary-backed skill (has ## Tool: entries but none + // are instruction-only) is already exposed as callable tools — skip + // it. A pure-frontmatter skill (no ## Tool: entries) is an + // instruction skill and is kept. + if len(entries) > 0 && len(provides) == 0 { + continue + } + + line := fmt.Sprintf("- %s: %s", loadName, desc) + if len(provides) > 0 { + line += fmt.Sprintf(" [provides: %s]", strings.Join(provides, ", ")) + } + // Note that skill uses cli_execute without listing specific + // binary names — the LLM already sees the allowed enum in the + // tool schema, and listing names here leaks internal tooling + // when users ask "what skills/tools do you have?" + if usesCLI { + line += " (uses cli_execute)" } + catalogEntries = append(catalogEntries, line) } if len(catalogEntries) == 0 { @@ -3193,7 +3255,8 @@ func (r *Runner) buildSkillCatalog() string { var b strings.Builder b.WriteString("## Available Skills\n\n") - b.WriteString("Use `read_skill` to load full instructions for a skill before using it.\n\n") + b.WriteString("To use a skill, call `read_skill` with the skill name (the identifier before the colon) to load its full instructions, then follow them. " + + "`provides:` lists the capabilities inside a skill — they are documentation loaded with the skill, not tools you call directly.\n\n") for _, entry := range catalogEntries { b.WriteString(entry) b.WriteString("\n") diff --git a/forge-cli/runtime/runner_skill_catalog_test.go b/forge-cli/runtime/runner_skill_catalog_test.go new file mode 100644 index 0000000..86fdef0 --- /dev/null +++ b/forge-cli/runtime/runner_skill_catalog_test.go @@ -0,0 +1,94 @@ +package runtime + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/initializ/forge/forge-core/tools/builtins" + "github.com/initializ/forge/forge-core/types" +) + +// writeCatalogSkill creates skills//SKILL.md with a frontmatter name +// plus a `## Tool:` heading whose name differs from the skill name — the +// exact shape that produced the read_skill lookup mismatch. +func writeCatalogSkill(t *testing.T, root, dir, name, toolHeading string) { + t.Helper() + d := filepath.Join(root, "skills", dir) + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + content := "---\nname: " + name + "\ndescription: Read-only kubectl triage.\n---\n" + + "## Tool: " + toolHeading + "\n\nDoes the triage.\n" + if err := os.WriteFile(filepath.Join(d, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// TestSkillCatalog_AdvertisesLoadableName asserts the catalog lists the +// skill by its loadable (frontmatter) name, not the internal `## Tool:` +// heading — the fix for the skill-lookup bug. +func TestSkillCatalog_AdvertisesLoadableName(t *testing.T) { + root := t.TempDir() + writeCatalogSkill(t, root, "k8s-incident-triage", "k8s-incident-triage", "k8s_triage") + + r := &Runner{cfg: RunnerConfig{WorkDir: root, Config: &types.ForgeConfig{AgentID: "test"}}} + cat := r.buildSkillCatalog() + + // The read_skill key (leading identifier) must be the loadable name. + if !strings.Contains(cat, "- k8s-incident-triage:") { + t.Errorf("catalog should key the line on the loadable name; got:\n%s", cat) + } + // The tool heading must NOT be advertised as a loadable skill (the bug). + if strings.Contains(cat, "- k8s_triage:") { + t.Errorf("catalog must not key a line on the internal tool name; got:\n%s", cat) + } + // But the tool name IS surfaced as a capability for tool selection. + if !strings.Contains(cat, "provides: k8s_triage") { + t.Errorf("catalog should surface the skill's tool names under provides; got:\n%s", cat) + } +} + +// TestSkillCatalog_NameIsReadSkillResolvable is the contract test that +// would have caught the original bug: every name the catalog advertises +// to the LLM must resolve through read_skill against the same workDir. +func TestSkillCatalog_NameIsReadSkillResolvable(t *testing.T) { + root := t.TempDir() + // Two skills, both with a `## Tool:` heading that differs from the + // skill name, and one whose directory differs from its name. + writeCatalogSkill(t, root, "k8s-incident-triage", "k8s-incident-triage", "k8s_triage") + writeCatalogSkill(t, root, "kube", "cluster-inspector", "inspect") + + r := &Runner{cfg: RunnerConfig{WorkDir: root, Config: &types.ForgeConfig{AgentID: "test"}}} + cat := r.buildSkillCatalog() + + readTool := builtins.NewReadSkillTool(root) + var checked int + for _, line := range strings.Split(cat, "\n") { + if !strings.HasPrefix(line, "- ") { + continue + } + // Extract the advertised name: "- : ". + name := strings.TrimSpace(strings.SplitN(strings.TrimPrefix(line, "- "), ":", 2)[0]) + if name == "" { + continue + } + out, err := readTool.Execute(context.Background(), + json.RawMessage(`{"name":`+mustQuote(name)+`}`)) + if err != nil { + t.Fatalf("read_skill(%q): %v", name, err) + } + if strings.Contains(out, "not found") { + t.Errorf("catalog advertised %q but read_skill could not resolve it: %s", name, out) + } + checked++ + } + if checked != 2 { + t.Fatalf("expected 2 advertised skills, checked %d\ncatalog:\n%s", checked, cat) + } +} + +func mustQuote(s string) string { b, _ := json.Marshal(s); return string(b) } diff --git a/forge-core/tools/builtins/read_skill.go b/forge-core/tools/builtins/read_skill.go index 8f282a6..6ee13ba 100644 --- a/forge-core/tools/builtins/read_skill.go +++ b/forge-core/tools/builtins/read_skill.go @@ -6,9 +6,11 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "github.com/initializ/forge/forge-core/tools" + skillparser "github.com/initializ/forge/forge-skills/parser" ) // ReadSkillTool reads a skill's SKILL.md file on demand. @@ -28,6 +30,7 @@ func (t *ReadSkillTool) Category() tools.Category { return tools.CategoryBuiltin func (t *ReadSkillTool) Description() string { return "Read the full instructions for an available skill. " + + "Pass the skill name exactly as listed under 'Available Skills'. " + "Returns the skill's SKILL.md content with usage details, parameters, and examples." } @@ -35,7 +38,7 @@ func (t *ReadSkillTool) InputSchema() json.RawMessage { return json.RawMessage(`{ "type": "object", "properties": { - "name": {"type": "string", "description": "Skill name (e.g. 'github', 'weather')"} + "name": {"type": "string", "description": "Skill name exactly as shown in the Available Skills list (e.g. 'k8s-incident-triage')"} }, "required": ["name"] }`) @@ -52,41 +55,123 @@ func (t *ReadSkillTool) Execute(_ context.Context, args json.RawMessage) (string return `{"error": "name is required"}`, nil } - // Security: prevent directory traversal - if strings.Contains(input.Name, "/") || strings.Contains(input.Name, "\\") || strings.Contains(input.Name, "..") { + // Security: prevent directory traversal. + if strings.ContainsAny(input.Name, `/\`) || strings.Contains(input.Name, "..") { return `{"error": "invalid skill name"}`, nil } - // Try multiple naming conventions to find the skill file. - // Tool names use underscores (k8s_triage) while directories use hyphens (k8s-incident-triage). - nameVariants := []string{input.Name} - hyphenated := strings.ReplaceAll(input.Name, "_", "-") - if hyphenated != input.Name { - nameVariants = append(nameVariants, hyphenated) + // 1) Fast path: direct filesystem lookup by the requested name and + // its underscore->hyphen variant. + if data, ok := t.readByName(input.Name); ok { + return string(data), nil } - var data []byte - var err error - for _, name := range nameVariants { + // 2) Index path: match the requested name against every skill's + // frontmatter `name` (the loadable identifier the catalog and the + // agent card advertise) and its directory/file name, normalized so + // case and underscore/hyphen differences don't matter. This resolves + // the name even when the skill directory differs from the skill's + // frontmatter name (e.g. tool advertised as "k8s-incident-triage" + // living in a directory of a different name). + index, available := t.buildNameIndex() + if path, ok := index[normalizeSkillKey(input.Name)]; ok { + if data, err := os.ReadFile(path); err == nil { //nolint:gosec // path derived from workDir scan, name sanitized above + return string(data), nil + } + } + + // 3) Not found — return the available skill names so the model can + // retry with a valid identifier instead of giving up. + if len(available) > 0 { + list, _ := json.Marshal(available) + return fmt.Sprintf(`{"error": "skill %q not found", "available_skills": %s}`, input.Name, list), nil + } + return fmt.Sprintf(`{"error": "skill %q not found"}`, input.Name), nil +} + +// readByName tries the two on-disk layouts for the given name and its +// underscore->hyphen variant. Returns (contents, true) on the first hit. +func (t *ReadSkillTool) readByName(name string) ([]byte, bool) { + variants := []string{name} + if hyphenated := strings.ReplaceAll(name, "_", "-"); hyphenated != name { + variants = append(variants, hyphenated) + } + for _, n := range variants { // Flat format: skills/{name}.md - path := filepath.Join(t.workDir, "skills", name+".md") - data, err = os.ReadFile(path) - if err == nil { - break + if data, err := os.ReadFile(filepath.Join(t.workDir, "skills", n+".md")); err == nil { //nolint:gosec // name sanitized by caller + return data, true } // Subdirectory format: skills/{name}/SKILL.md - path = filepath.Join(t.workDir, "skills", name, "SKILL.md") - data, err = os.ReadFile(path) - if err == nil { - break + if data, err := os.ReadFile(filepath.Join(t.workDir, "skills", n, "SKILL.md")); err == nil { //nolint:gosec // name sanitized by caller + return data, true } } + return nil, false +} + +// buildNameIndex scans the skills directory and returns: +// - a map from normalized skill key -> SKILL.md path, keyed by BOTH the +// frontmatter `name` and the directory/file name of each skill; +// - a sorted, de-duplicated list of the loadable skill names for the +// "not found" hint (frontmatter name preferred, else directory/file). +func (t *ReadSkillTool) buildNameIndex() (map[string]string, []string) { + index := make(map[string]string) + displaySet := make(map[string]struct{}) + skillsDir := filepath.Join(t.workDir, "skills") + ents, err := os.ReadDir(skillsDir) if err != nil { - if os.IsNotExist(err) { - return fmt.Sprintf(`{"error": "skill %q not found"}`, input.Name), nil + return index, nil + } + add := func(path, dirOrBase string) { + display := dirOrBase + if name := frontmatterName(path); name != "" { + index[normalizeSkillKey(name)] = path + display = name } - return "", fmt.Errorf("reading skill file: %w", err) + index[normalizeSkillKey(dirOrBase)] = path + displaySet[display] = struct{}{} } + for _, e := range ents { + if e.IsDir() { + p := filepath.Join(skillsDir, e.Name(), "SKILL.md") + if _, statErr := os.Stat(p); statErr == nil { + add(p, e.Name()) + } + continue + } + // Flat format: skills/{name}.md (skip the scripts/ helper dir). + if strings.HasSuffix(e.Name(), ".md") { + p := filepath.Join(skillsDir, e.Name()) + add(p, strings.TrimSuffix(e.Name(), ".md")) + } + } + display := make([]string, 0, len(displaySet)) + for name := range displaySet { + display = append(display, name) + } + sort.Strings(display) + return index, display +} + +// frontmatterName parses just the frontmatter `name` from a SKILL.md +// file. Returns "" when the file can't be read or has no name. +func frontmatterName(path string) string { + f, err := os.Open(path) //nolint:gosec // path derived from workDir scan + if err != nil { + return "" + } + defer func() { _ = f.Close() }() + _, meta, err := skillparser.ParseWithMetadata(f) + if err != nil || meta == nil { + return "" + } + return strings.TrimSpace(meta.Name) +} - return string(data), nil +// normalizeSkillKey lowercases and unifies underscore/hyphen so that +// "K8s_Triage", "k8s-triage" and "k8s_triage" all collide. It does NOT +// bridge semantically different names — the catalog advertises the exact +// loadable name, and this only absorbs case/separator drift from the LLM. +func normalizeSkillKey(s string) string { + return strings.ReplaceAll(strings.ToLower(strings.TrimSpace(s)), "_", "-") } diff --git a/forge-core/tools/builtins/read_skill_test.go b/forge-core/tools/builtins/read_skill_test.go new file mode 100644 index 0000000..8639eaa --- /dev/null +++ b/forge-core/tools/builtins/read_skill_test.go @@ -0,0 +1,106 @@ +package builtins + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// writeSkill creates skills//SKILL.md under root with the given +// frontmatter name + body, and returns root. +func writeSkill(t *testing.T, root, dir, frontmatterName, body string) { + t.Helper() + skillDir := filepath.Join(root, "skills", dir) + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + content := "---\nname: " + frontmatterName + "\ndescription: demo\n---\n" + body + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func readSkill(t *testing.T, root, name string) string { + t.Helper() + out, err := NewReadSkillTool(root).Execute(context.Background(), + json.RawMessage(`{"name":`+mustJSON(name)+`}`)) + if err != nil { + t.Fatalf("Execute(%q): %v", name, err) + } + return out +} + +func mustJSON(s string) string { b, _ := json.Marshal(s); return string(b) } + +// TestReadSkill_ResolvesByFrontmatterNameWhenDirDiffers is the core +// regression for the skill-lookup bug: the loadable name advertised to +// the LLM (the frontmatter name) must resolve even when the skill +// directory is named differently. +func TestReadSkill_ResolvesByFrontmatterNameWhenDirDiffers(t *testing.T) { + root := t.TempDir() + // Directory "kube" but frontmatter name "k8s-incident-triage". + writeSkill(t, root, "kube", "k8s-incident-triage", "# Triage\nRead-only kubectl triage.\n") + + // The name the catalog advertises (frontmatter name) must resolve. + if out := readSkill(t, root, "k8s-incident-triage"); strings.Contains(out, "not found") { + t.Errorf("frontmatter name did not resolve: %s", out) + } + // The directory name must still resolve (back-compat). + if out := readSkill(t, root, "kube"); strings.Contains(out, "not found") { + t.Errorf("directory name did not resolve: %s", out) + } +} + +// TestReadSkill_NormalizedMatch — case and underscore/hyphen drift from +// the model must not break resolution. +func TestReadSkill_NormalizedMatch(t *testing.T) { + root := t.TempDir() + writeSkill(t, root, "k8s-incident-triage", "k8s-incident-triage", "# body\n") + for _, variant := range []string{"k8s_incident_triage", "K8S-Incident-Triage", "k8s-incident-triage"} { + if out := readSkill(t, root, variant); strings.Contains(out, "not found") { + t.Errorf("variant %q did not resolve: %s", variant, out) + } + } +} + +// TestReadSkill_FlatFormat — skills/.md layout still resolves. +func TestReadSkill_FlatFormat(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "skills"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "skills", "weather.md"), []byte("---\nname: weather\ndescription: d\n---\nbody"), 0o644); err != nil { + t.Fatal(err) + } + if out := readSkill(t, root, "weather"); strings.Contains(out, "not found") { + t.Errorf("flat skill did not resolve: %s", out) + } +} + +// TestReadSkill_NotFoundListsAvailable — a miss returns the loadable +// names so the model can retry instead of giving up. +func TestReadSkill_NotFoundListsAvailable(t *testing.T) { + root := t.TempDir() + writeSkill(t, root, "kube", "k8s-incident-triage", "# body\n") + out := readSkill(t, root, "nonexistent") + if !strings.Contains(out, "not found") { + t.Fatalf("expected not-found, got %s", out) + } + if !strings.Contains(out, "available_skills") || !strings.Contains(out, "k8s-incident-triage") { + t.Errorf("not-found should list available skills incl. the frontmatter name: %s", out) + } +} + +// TestReadSkill_TraversalRejected keeps the directory-traversal guard. +func TestReadSkill_TraversalRejected(t *testing.T) { + root := t.TempDir() + for _, bad := range []string{"../etc/passwd", "a/b", `a\b`, ".."} { + out := readSkill(t, root, bad) + if !strings.Contains(out, "invalid skill name") { + t.Errorf("traversal %q not rejected: %s", bad, out) + } + } +} From 46b91a3bec41f574b173d356d9f141a71d66f598 Mon Sep 17 00:00:00 2001 From: MK Date: Mon, 6 Jul 2026 23:40:31 -0400 Subject: [PATCH 2/2] feat(skills): read_skill surfaces skill directory files (scripts + reference) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skills ship more than SKILL.md — helper scripts (shell, python, javascript), reference folders, and additional markdown all live in the skill's directory and reach the running agent via COPY . . at build time, but were invisible to the LLM because read_skill returned only SKILL.md. read_skill now appends a '## Skill files' listing of every other file in a subdirectory skill's folder, relative to the agent root and annotated with the script language, so the model can read or execute them as the skill's steps describe. Flat single-file skills are unaffected. Also documents why skillEntryHasScript (the catalog 'provides' exclusion) stays aligned to registerSkillTools' .sh-only registration: a py/js tool is not yet a callable tool, so it correctly stays in 'provides' and its script is surfaced by the new file listing. Broadening the extension set must wait for registerSkillTools to register those languages, or the tools would vanish from both the tool list and the catalog. --- forge-cli/runtime/runner.go | 15 +- forge-core/tools/builtins/read_skill.go | 142 ++++++++++++++----- forge-core/tools/builtins/read_skill_test.go | 56 ++++++++ 3 files changed, 177 insertions(+), 36 deletions(-) diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index a470d6c..3ef37b9 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -3123,9 +3123,18 @@ func (r *Runner) buildSystemPrompt() string { // (those without scripts) for the system prompt. Script-backed skills are // already registered as first-class tools and don't need catalog entries. // skillEntryHasScript reports whether a `## Tool:` entry is backed by a -// script on disk, mirroring registerSkillTools' lookup. A script-backed -// entry is registered as a first-class callable tool, so it is excluded -// from the read_skill catalog's "provides" list. +// script that registerSkillTools actually registers as a first-class +// callable tool — currently only `scripts/.sh`. Such tools are +// excluded from the read_skill catalog's "provides" list because the LLM +// invokes them directly by name. +// +// This deliberately mirrors registerSkillTools' `.sh`-only lookup, NOT the +// full set of script languages. A tool backed by a `.py`/`.js` script is +// not yet registered as a callable tool, so it stays in "provides" (the +// LLM reaches it by loading the skill), and read_skill's file listing +// surfaces the actual script. Keep this in lockstep with +// registerSkillTools: if it learns to register other script languages, +// broaden this check at the same time or those tools vanish from both. func (r *Runner) skillEntryHasScript(skillDir, toolName string) bool { scriptName := strings.ReplaceAll(toolName, "_", "-") if skillDir != "" { diff --git a/forge-core/tools/builtins/read_skill.go b/forge-core/tools/builtins/read_skill.go index 6ee13ba..c61b35b 100644 --- a/forge-core/tools/builtins/read_skill.go +++ b/forge-core/tools/builtins/read_skill.go @@ -60,53 +60,129 @@ func (t *ReadSkillTool) Execute(_ context.Context, args json.RawMessage) (string return `{"error": "invalid skill name"}`, nil } - // 1) Fast path: direct filesystem lookup by the requested name and - // its underscore->hyphen variant. - if data, ok := t.readByName(input.Name); ok { - return string(data), nil - } - - // 2) Index path: match the requested name against every skill's - // frontmatter `name` (the loadable identifier the catalog and the - // agent card advertise) and its directory/file name, normalized so - // case and underscore/hyphen differences don't matter. This resolves - // the name even when the skill directory differs from the skill's - // frontmatter name (e.g. tool advertised as "k8s-incident-triage" - // living in a directory of a different name). - index, available := t.buildNameIndex() - if path, ok := index[normalizeSkillKey(input.Name)]; ok { - if data, err := os.ReadFile(path); err == nil { //nolint:gosec // path derived from workDir scan, name sanitized above - return string(data), nil + // Resolve the SKILL.md path: direct filesystem lookup first, then a + // frontmatter-name index so the requested name resolves even when the + // skill directory differs from the skill's frontmatter name. + path := t.resolvePath(input.Name) + if path == "" { + // Not found — return the available skill names so the model can + // retry with a valid identifier instead of giving up. + if _, available := t.buildNameIndex(); len(available) > 0 { + list, _ := json.Marshal(available) + return fmt.Sprintf(`{"error": "skill %q not found", "available_skills": %s}`, input.Name, list), nil } + return fmt.Sprintf(`{"error": "skill %q not found"}`, input.Name), nil } - // 3) Not found — return the available skill names so the model can - // retry with a valid identifier instead of giving up. - if len(available) > 0 { - list, _ := json.Marshal(available) - return fmt.Sprintf(`{"error": "skill %q not found", "available_skills": %s}`, input.Name, list), nil + data, err := os.ReadFile(path) //nolint:gosec // path derived from workDir scan / sanitized name + if err != nil { + return fmt.Sprintf(`{"error": "skill %q not found"}`, input.Name), nil + } + content := string(data) + + // Surface the rest of the skill's directory — helper scripts (shell, + // python, javascript, ...), reference material, and additional + // markdown all live alongside SKILL.md and reach the running agent + // (COPY . . at build time), but are invisible unless listed. The + // model can then read or execute them as the skill's steps describe. + if footer := t.skillFilesFooter(path); footer != "" { + content += "\n\n" + footer } - return fmt.Sprintf(`{"error": "skill %q not found"}`, input.Name), nil + return content, nil } -// readByName tries the two on-disk layouts for the given name and its -// underscore->hyphen variant. Returns (contents, true) on the first hit. -func (t *ReadSkillTool) readByName(name string) ([]byte, bool) { +// resolvePath returns the SKILL.md path for the requested name, or "" if +// no skill resolves. Tries the on-disk layouts (and the underscore->hyphen +// variant) first, then the normalized frontmatter-name index. +func (t *ReadSkillTool) resolvePath(name string) string { variants := []string{name} if hyphenated := strings.ReplaceAll(name, "_", "-"); hyphenated != name { variants = append(variants, hyphenated) } for _, n := range variants { - // Flat format: skills/{name}.md - if data, err := os.ReadFile(filepath.Join(t.workDir, "skills", n+".md")); err == nil { //nolint:gosec // name sanitized by caller - return data, true + if p := filepath.Join(t.workDir, "skills", n+".md"); fileExists(p) { + return p + } + if p := filepath.Join(t.workDir, "skills", n, "SKILL.md"); fileExists(p) { + return p + } + } + if index, _ := t.buildNameIndex(); index != nil { + if p, ok := index[normalizeSkillKey(name)]; ok { + return p + } + } + return "" +} + +func fileExists(p string) bool { + info, err := os.Stat(p) + return err == nil && !info.IsDir() +} + +// scriptLang maps a file extension to the interpreter language, for +// annotating helper scripts in the file listing. Mirrors the languages +// forge recognizes for tool scripts. +func scriptLang(ext string) string { + switch strings.ToLower(ext) { + case ".sh", ".bash": + return "shell" + case ".py": + return "python" + case ".js": + return "javascript" + case ".ts": + return "typescript" + default: + return "" + } +} + +// skillFilesFooter lists the other files living in a subdirectory skill's +// folder (everything except the SKILL.md itself), so the model knows the +// skill ships helper scripts and reference material it can read or run. +// Paths are relative to the agent working directory so the model can pass +// them straight to file tools. Returns "" for flat (single-file) skills +// or when the folder holds nothing else. +func (t *ReadSkillTool) skillFilesFooter(skillMdPath string) string { + if filepath.Base(skillMdPath) != "SKILL.md" { + return "" // flat skills/.md layout has no companion directory + } + dir := filepath.Dir(skillMdPath) + var files []string + const maxFiles = 200 + _ = filepath.WalkDir(dir, func(p string, d os.DirEntry, err error) error { + if err != nil || d.IsDir() || len(files) >= maxFiles { + return nil + } + if p == skillMdPath { + return nil + } + rel, relErr := filepath.Rel(t.workDir, p) + if relErr != nil { + rel = p } - // Subdirectory format: skills/{name}/SKILL.md - if data, err := os.ReadFile(filepath.Join(t.workDir, "skills", n, "SKILL.md")); err == nil { //nolint:gosec // name sanitized by caller - return data, true + if lang := scriptLang(filepath.Ext(p)); lang != "" { + files = append(files, rel+" — "+lang) + } else { + files = append(files, rel) } + return nil + }) + if len(files) == 0 { + return "" + } + sort.Strings(files) + var b strings.Builder + b.WriteString("## Skill files\n") + b.WriteString("This skill ships the following supporting files (relative to the agent root). ") + b.WriteString("Read or execute them via your file/exec tools as the steps above describe:\n") + for _, f := range files { + b.WriteString("- ") + b.WriteString(f) + b.WriteString("\n") } - return nil, false + return b.String() } // buildNameIndex scans the skills directory and returns: diff --git a/forge-core/tools/builtins/read_skill_test.go b/forge-core/tools/builtins/read_skill_test.go index 8639eaa..d91b48e 100644 --- a/forge-core/tools/builtins/read_skill_test.go +++ b/forge-core/tools/builtins/read_skill_test.go @@ -94,6 +94,62 @@ func TestReadSkill_NotFoundListsAvailable(t *testing.T) { } } +// TestReadSkill_ListsSkillFiles — loading a skill surfaces its helper +// scripts (any language) and reference material so the model knows they +// exist and can read/run them. +func TestReadSkill_ListsSkillFiles(t *testing.T) { + root := t.TempDir() + writeSkill(t, root, "k8s-incident-triage", "k8s-incident-triage", "# body\n") + base := filepath.Join(root, "skills", "k8s-incident-triage") + for _, f := range []struct{ rel, body string }{ + {"scripts/triage.py", "print('x')"}, + {"scripts/collect.sh", "echo x"}, + {"scripts/render.js", "console.log(1)"}, + {"reference/runbook.md", "# runbook"}, + {"reference/slo.yaml", "target: 0.99"}, + } { + p := filepath.Join(base, f.rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(f.body), 0o644); err != nil { + t.Fatal(err) + } + } + out := readSkill(t, root, "k8s-incident-triage") + for _, want := range []string{ + "## Skill files", + "skills/k8s-incident-triage/scripts/triage.py — python", + "skills/k8s-incident-triage/scripts/collect.sh — shell", + "skills/k8s-incident-triage/scripts/render.js — javascript", + "skills/k8s-incident-triage/reference/runbook.md", + "skills/k8s-incident-triage/reference/slo.yaml", + } { + if !strings.Contains(out, want) { + t.Errorf("skill files listing missing %q\nfull:\n%s", want, out) + } + } + // SKILL.md itself must not be listed. + if strings.Contains(out, "skills/k8s-incident-triage/SKILL.md") { + t.Errorf("SKILL.md should not be listed among skill files:\n%s", out) + } +} + +// TestReadSkill_FlatFormatNoFileListing — a flat skills/.md skill +// has no companion directory, so no file listing is appended. +func TestReadSkill_FlatFormatNoFileListing(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "skills"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "skills", "weather.md"), []byte("---\nname: weather\ndescription: d\n---\nbody"), 0o644); err != nil { + t.Fatal(err) + } + if out := readSkill(t, root, "weather"); strings.Contains(out, "## Skill files") { + t.Errorf("flat skill should have no file listing: %s", out) + } +} + // TestReadSkill_TraversalRejected keeps the directory-traversal guard. func TestReadSkill_TraversalRejected(t *testing.T) { root := t.TempDir()