From 147e26ac602e0dffbbdbc0c802a84cba8c0f2f53 Mon Sep 17 00:00:00 2001
From: admin-mf <admin@magicform.ai>
Date: Fri, 6 Mar 2026 00:08:48 -0600
Subject: [PATCH] security(skills): prevent workspace skills from shadowing
 builtins
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Builtin skill names are now reserved — workspace and global skills
with the same name as a builtin are skipped with a warning log.
LoadSkill also checks builtins first. This prevents supply chain
attacks where a malicious skill replaces a trusted builtin.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 pkg/skills/loader.go      | 52 ++++++++++++++++++++++++++++++++-------
 pkg/skills/loader_test.go |  6 ++---
 2 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/pkg/skills/loader.go b/pkg/skills/loader.go
index b9c666c35..7323d6686 100644
--- a/pkg/skills/loader.go
+++ b/pkg/skills/loader.go
@@ -96,9 +96,37 @@ func NewSkillsLoader(workspace string, globalSkills string, builtinSkills string
 	}
 }
 
+// builtinSkillNames returns the set of validated skill names from the builtin directory.
+func (sl *SkillsLoader) builtinSkillNames() map[string]bool {
+	names := make(map[string]bool)
+	if sl.builtinSkills == "" {
+		return names
+	}
+	dirs, err := os.ReadDir(sl.builtinSkills)
+	if err != nil {
+		return names
+	}
+	for _, d := range dirs {
+		if !d.IsDir() {
+			continue
+		}
+		skillFile := filepath.Join(sl.builtinSkills, d.Name(), "SKILL.md")
+		if _, err := os.Stat(skillFile); err != nil {
+			continue
+		}
+		name := d.Name()
+		if metadata := sl.getSkillMetadata(skillFile); metadata != nil && metadata.Name != "" {
+			name = metadata.Name
+		}
+		names[name] = true
+	}
+	return names
+}
+
 func (sl *SkillsLoader) ListSkills() []SkillInfo {
 	skills := make([]SkillInfo, 0)
 	seen := make(map[string]bool)
+	builtinNames := sl.builtinSkillNames()
 
 	addSkills := func(dir, source string) {
 		if dir == "" {
@@ -133,6 +161,12 @@ func (sl *SkillsLoader) ListSkills() []SkillInfo {
 			if seen[info.Name] {
 				continue
 			}
+			// Block workspace/global skills from shadowing builtins.
+			if source != "builtin" && builtinNames[info.Name] {
+				slog.Warn("skill shadows a builtin and will be skipped",
+					"name", info.Name, "source", source)
+				continue
+			}
 			seen[info.Name] = true
 			skills = append(skills, info)
 		}
@@ -147,7 +181,15 @@ func (sl *SkillsLoader) ListSkills() []SkillInfo {
 }
 
 func (sl *SkillsLoader) LoadSkill(name string) (string, bool) {
-	// 1. load from workspace skills first (project-level)
+	// If this is a builtin skill, always load from builtin to prevent shadowing.
+	if sl.builtinSkills != "" {
+		skillFile := filepath.Join(sl.builtinSkills, name, "SKILL.md")
+		if content, err := os.ReadFile(skillFile); err == nil {
+			return sl.stripFrontmatter(string(content)), true
+		}
+	}
+
+	// 1. load from workspace skills (project-level)
 	if sl.workspaceSkills != "" {
 		skillFile := filepath.Join(sl.workspaceSkills, name, "SKILL.md")
 		if content, err := os.ReadFile(skillFile); err == nil {
@@ -163,14 +205,6 @@ func (sl *SkillsLoader) LoadSkill(name string) (string, bool) {
 		}
 	}
 
-	// 3. finally load from builtin skills
-	if sl.builtinSkills != "" {
-		skillFile := filepath.Join(sl.builtinSkills, name, "SKILL.md")
-		if content, err := os.ReadFile(skillFile); err == nil {
-			return sl.stripFrontmatter(string(content)), true
-		}
-	}
-
 	return "", false
 }
 
diff --git a/pkg/skills/loader_test.go b/pkg/skills/loader_test.go
index 31619f9c2..5cc28fda8 100644
--- a/pkg/skills/loader_test.go
+++ b/pkg/skills/loader_test.go
@@ -163,7 +163,7 @@ func TestListSkillsWorkspaceOverridesGlobal(t *testing.T) {
 	assert.Equal(t, "workspace version", skills[0].Description)
 }
 
-func TestListSkillsGlobalOverridesBuiltin(t *testing.T) {
+func TestListSkillsBuiltinCannotBeShadowed(t *testing.T) {
 	tmp := t.TempDir()
 	ws := filepath.Join(tmp, "workspace")
 	global := filepath.Join(tmp, "global")
@@ -176,8 +176,8 @@ func TestListSkillsGlobalOverridesBuiltin(t *testing.T) {
 	skills := sl.ListSkills()
 
 	assert.Len(t, skills, 1)
-	assert.Equal(t, "global", skills[0].Source)
-	assert.Equal(t, "global version", skills[0].Description)
+	assert.Equal(t, "builtin", skills[0].Source)
+	assert.Equal(t, "builtin version", skills[0].Description)
 }
 
 func TestListSkillsMetadataNameDedup(t *testing.T) {