diff --git a/server/server.go b/server/server.go index 1eae004..5cbcf0b 100644 --- a/server/server.go +++ b/server/server.go @@ -219,9 +219,10 @@ func New(ctx context.Context, m *Manager, skillDir string, dataDir string, keyDi } wireAgentAugment(m.pg, s.skillDir, s.hostTools) // wire visible skills/MCP + traffic/orchestration host tools into the agent toolset domainReg := buildDomainReg(m.Assets()) - wireTools(m.pg, domainReg) // built-in tools table: filter per agent + override description/schema + inject defaults - seedPrompts(m.pg) // seed built-in agent default prompt bodies into agent_prompts (only when empty) - s.seedOrchestrationTools() // seed P2 cross-task orchestration tools into the tools table (bindable per agent) + wireTools(m.pg, domainReg) // built-in tools table: filter per agent + override description/schema + inject defaults + seedPrompts(m.pg) // seed built-in agent default prompt bodies into agent_prompts (only when empty) + warnBrokenSkills(s.skillDir) // surface skills whose frontmatter silently failed to parse + s.seedOrchestrationTools() // seed P2 cross-task orchestration tools into the tools table (bindable per agent) if err := s.seedFindingRetester(); err != nil { log.Printf("[retester] seed: %v", err) } diff --git a/server/server_mgmt.go b/server/server_mgmt.go index d1fa594..2d2e602 100644 --- a/server/server_mgmt.go +++ b/server/server_mgmt.go @@ -1108,17 +1108,21 @@ func (s *Server) fsCreateSkill(w http.ResponseWriter, r *http.Request) { // Markdown body: step-by-step instructions var sb strings.Builder sb.WriteString("---\n") - fmt.Fprintf(&sb, "name: %s\n", body.Name) - fmt.Fprintf(&sb, "description: %s\n", body.Description) + fmt.Fprintf(&sb, "name: %s\n", yamlScalar(body.Name)) + fmt.Fprintf(&sb, "description: %s\n", yamlScalar(body.Description)) if body.License != "" { - fmt.Fprintf(&sb, "license: %s\n", body.License) + fmt.Fprintf(&sb, "license: %s\n", yamlScalar(body.License)) } if body.Compatibility != "" { - fmt.Fprintf(&sb, "compatibility: %s\n", body.Compatibility) + fmt.Fprintf(&sb, "compatibility: %s\n", yamlScalar(body.Compatibility)) } // mcps: MCP servers this skill unlocks on load (skill-gated deferred tools). if mcps := cleanStrs(body.MCPs); len(mcps) > 0 { - fmt.Fprintf(&sb, "mcps: %s\n", strings.Join(mcps, ", ")) + quoted := make([]string, len(mcps)) + for i, m := range mcps { + quoted[i] = yamlScalar(m) + } + fmt.Fprintf(&sb, "mcps: %s\n", strings.Join(quoted, ", ")) } sb.WriteString("---\n") if strings.TrimSpace(body.Instructions) != "" { @@ -1134,6 +1138,14 @@ func (s *Server) fsCreateSkill(w http.ResponseWriter, r *http.Request) { writeErr(w, 500, err.Error()) return } + // Verify with the real parser: a description containing ": " (or any value that + // is not plain-safe YAML) used to break the whole frontmatter silently, leaving + // a skill whose fields were all empty and whose name came from the directory. + if err := verifySkillFile(skillPath, body.Name, cleanStrs(body.MCPs)); err != nil { + _ = os.RemoveAll(skillPath) + writeErr(w, 400, "generated SKILL.md does not round-trip through the skill parser: "+err.Error()) + return + } writeJSON(w, 201, map[string]any{"name": body.Name}) } @@ -1176,6 +1188,54 @@ func (s *Server) fsUpdateSkillMeta(w http.ResponseWriter, r *http.Request) { writeJSON(w, 200, map[string]any{"ok": true}) } +// yamlScalar renders one frontmatter scalar so the emitted YAML is valid by +// construction: plain-safe tokens stay bare, everything else is single-quoted +// with internal quotes doubled (YAML single-quote escaping). +func yamlScalar(s string) string { + if s != "" && !strings.ContainsAny(s, ":#{}[]&*!|>'\"%@`,\n") && !strings.HasPrefix(s, " ") && !strings.HasSuffix(s, " ") { + return s + } + return "'" + strings.ReplaceAll(s, "'", "''") + "'" +} + +// verifySkillFile re-loads a written SKILL.md with the real skill parser and +// asserts the name and the mcps list survived the round trip. +func verifySkillFile(skillPath, wantName string, wantMCPs []string) error { + reg, err := skill.LoadDir(skillPath) + if err != nil { + return fmt.Errorf("load: %w", err) + } + for _, s := range reg.List() { + if s.Dir == "" { + continue + } + if s.Name != wantName { + return fmt.Errorf("parsed name %q != written %q (frontmatter did not parse; the name fell back to the directory)", s.Name, wantName) + } + if len(wantMCPs) > 0 && len(s.MCPs) != len(wantMCPs) { + return fmt.Errorf("parsed mcps %v != written %v", s.MCPs, wantMCPs) + } + return nil + } + return fmt.Errorf("no skill loaded from %s", skillPath) +} + +// warnBrokenSkills logs skills whose parsed name does not match their directory +// name: the signature of frontmatter that failed to parse and silently fell back +// (fields empty, mcps gate lost). Best-effort, startup only. +func warnBrokenSkills(dir string) { + reg, err := skill.LoadDir(dir) + if err != nil { + return + } + for _, s := range reg.List() { + if s.Dir == "" || s.Name == filepath.Base(s.Dir) { + continue + } + log.Printf("[skill] %s: frontmatter did not parse (loaded under the directory name; description and mcps are empty) - fix the YAML in %s", s.Name, filepath.Join(s.Dir, "SKILL.md")) + } +} + // rewriteSkillFrontmatter parses the SKILL.md YAML frontmatter and replaces the // fields given as non-nil pointers. The instruction body (after the closing ---) is // preserved verbatim. Returns an error if the file has no recognisable frontmatter. @@ -1211,11 +1271,11 @@ func rewriteSkillFrontmatter(content []byte, mcps *[]string, description, licens } for i, p := range pairs { if p.k == key { - pairs[i].v = strings.TrimSpace(*val) + pairs[i].v = yamlScalar(strings.TrimSpace(*val)) return } } - pairs = append(pairs, kv{key, strings.TrimSpace(*val)}) + pairs = append(pairs, kv{key, yamlScalar(strings.TrimSpace(*val))}) } applyStr("description", description) applyStr("license", license) @@ -1231,7 +1291,11 @@ func rewriteSkillFrontmatter(content []byte, mcps *[]string, description, licens } pairs = filtered if len(cleaned) > 0 { - pairs = append(pairs, kv{"mcps", strings.Join(cleaned, ", ")}) + quoted := make([]string, len(cleaned)) + for i, m := range cleaned { + quoted[i] = yamlScalar(m) + } + pairs = append(pairs, kv{"mcps", strings.Join(quoted, ", ")}) } } // Reconstruct diff --git a/server/skill_frontmatter_test.go b/server/skill_frontmatter_test.go new file mode 100644 index 0000000..a9f6a7b --- /dev/null +++ b/server/skill_frontmatter_test.go @@ -0,0 +1,72 @@ +package server + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Autumn-27/norma/skill" +) + +func TestYamlScalar(t *testing.T) { + cases := []struct{ in, want string }{ + {"rea", "rea"}, + {"api-recon", "api-recon"}, + {"plain value with spaces", "plain value with spaces"}, + {"target: embedded", "'target: embedded'"}, + {"it's quoted", "'it''s quoted'"}, + {"ends #now", "'ends #now'"}, + {"comma, separated", "'comma, separated'"}, + {" leading", "' leading'"}, + {"", "''"}, + } + for _, c := range cases { + if got := yamlScalar(c.in); got != c.want { + t.Errorf("yamlScalar(%q) = %q, want %q", c.in, got, c.want) + } + } +} + +// A SKILL.md written the way fsCreateSkill now writes it (yamlScalar values) +// must round-trip through the real parser with name and mcps intact, even with +// a colon+space and quotes in the description. The pre-fix format (raw +// interpolation) fails exactly there and falls back to the directory name. +func TestSkillFrontmatterRoundTrip(t *testing.T) { + dir := t.TempDir() + skillDir := filepath.Join(dir, "colon-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + desc := "Analyze target: embedded secrets, it's binary" + content := fmt.Sprintf("---\nname: %s\ndescription: %s\nmcps: %s\n---\nbody instructions\n", + yamlScalar("colon-skill"), yamlScalar(desc), yamlScalar("rea")) + + skillMD := filepath.Join(skillDir, "SKILL.md") + if err := os.WriteFile(skillMD, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + if err := verifySkillFile(skillDir, "colon-skill", []string{"rea"}); err != nil { + t.Fatalf("safe format failed verification: %v", err) + } + reg, err := skill.LoadDir(skillDir) + if err != nil || len(reg.List()) != 1 { + t.Fatalf("LoadDir: %v (%d skills)", err, len(reg.List())) + } + s := reg.List()[0] + if s.Name != "colon-skill" || s.Description != desc || len(s.MCPs) != 1 || s.MCPs[0] != "rea" { + t.Fatalf("round-trip mismatch: name=%q desc=%q mcps=%v", s.Name, s.Description, s.MCPs) + } + + // the old, unsafe emission: unquoted colon+space breaks the whole frontmatter + unsafe := fmt.Sprintf("---\nname: %s\ndescription: %s\nmcps: rea\n---\nbody\n", "colon-skill", desc) + if err := os.WriteFile(skillMD, []byte(unsafe), 0o644); err != nil { + t.Fatal(err) + } + if err := verifySkillFile(skillDir, "colon-skill", []string{"rea"}); err == nil { + t.Fatal("unsafe format unexpectedly verified") + } else if !strings.Contains(err.Error(), "fell back") { + t.Fatalf("unexpected error: %v", err) + } +} diff --git a/skills/scopesentry/SKILL.md b/skills/scopesentry/SKILL.md index e4c5487..1a23950 100644 --- a/skills/scopesentry/SKILL.md +++ b/skills/scopesentry/SKILL.md @@ -1,6 +1,6 @@ --- -## name: scopesentry-mcp +name: scopesentry-mcp description: Manage the security scanning platform (projects, tasks, templates, assets, nodes) through the ScopeSentry MCP. Use when the user mentions ScopeSentry, MCP, API Key, scan tasks, or asset queries. # ScopeSentry MCP usage guide