From 242f9919a41292cab79ff449531de3a06d9c6152 Mon Sep 17 00:00:00 2001 From: jack Date: Mon, 27 Jul 2026 00:11:26 +0800 Subject: [PATCH] fix(skills): reserve cloud-managed providers --- internal/skills/managed_skills_test.go | 239 +++++++++++++++++++++++++ internal/skills/skills.go | 142 +++++++++++++-- 2 files changed, 366 insertions(+), 15 deletions(-) create mode 100644 internal/skills/managed_skills_test.go diff --git a/internal/skills/managed_skills_test.go b/internal/skills/managed_skills_test.go new file mode 100644 index 00000000..5571c67b --- /dev/null +++ b/internal/skills/managed_skills_test.go @@ -0,0 +1,239 @@ +package skills + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestManagedSkillsHaveHighestPrecedenceByParsedName(t *testing.T) { + home := t.TempDir() + project := t.TempDir() + managed := filepath.Join(t.TempDir(), "managed") + t.Setenv("HOME", home) + t.Setenv(EnvManagedSkillsDir, managed) + t.Setenv(EnvReservedSkills, "github, gitlab, gitea") + + // Same directory name and frontmatter name across every lower-priority + // source: managed must win. + writeTestSkill(t, filepath.Join(home, ".agents", "skills"), "github", "github", "agents github") + writeTestSkill(t, filepath.Join(home, ".jcode", "skills"), "github", "github", "user github") + writeTestSkill(t, filepath.Join(project, ".jcode", "skills"), "github", "github", "project github") + writeTestSkill(t, managed, "github", "github", "managed github") + + // Different directory names still collide by parsed frontmatter Name. + writeTestSkill(t, filepath.Join(home, ".agents", "skills"), "agent-gitlab", "gitlab", "agents gitlab") + writeTestSkill(t, filepath.Join(home, ".jcode", "skills"), "user-gitlab", "gitlab", "user gitlab") + writeTestSkill(t, filepath.Join(project, ".jcode", "skills"), "project-gitlab", "gitlab", "project gitlab") + writeTestSkill(t, managed, "provider-gitlab", "gitlab", "managed gitlab") + + // A reserved definition that is not selected into the managed root must + // disappear instead of falling back to an untrusted source. + writeTestSkill(t, filepath.Join(home, ".agents", "skills"), "gitea", "gitea", "agents gitea") + writeTestSkill(t, filepath.Join(home, ".jcode", "skills"), "gitea", "gitea", "user gitea") + writeTestSkill(t, filepath.Join(project, ".jcode", "skills"), "gitea", "gitea", "project gitea") + + loader := NewLoader() + loader.ScanProjectSkills(project) + + assertSkillBody(t, loader, "github", "managed github", "managed") + assertSkillBody(t, loader, "gitlab", "managed gitlab", "managed") + if got := loader.Get("gitea"); got != nil { + t.Fatalf("unselected reserved skill loaded from %s: %#v", got.Source, got) + } + if strings.Contains(loader.Descriptions(), "gitea") { + t.Fatalf("unselected reserved skill advertised: %q", loader.Descriptions()) + } + for _, skill := range loader.SlashCommands() { + if skill.Name == "gitea" { + t.Fatalf("unselected reserved skill exposed as slash command: %#v", skill) + } + } +} + +func TestManagedSkillsPreserveNonReservedOverrideChain(t *testing.T) { + home := t.TempDir() + project := t.TempDir() + managed := filepath.Join(t.TempDir(), "managed") + t.Setenv("HOME", home) + t.Setenv(EnvManagedSkillsDir, managed) + t.Setenv(EnvReservedSkills, "github gitlab gitea") + + writeTestSkill(t, filepath.Join(home, ".agents", "skills"), "custom", "custom", "agents custom") + writeTestSkill(t, filepath.Join(home, ".jcode", "skills"), "custom-user-dir", "custom", "user custom") + writeTestSkill(t, filepath.Join(project, ".jcode", "skills"), "custom-project-dir", "custom", "project custom") + writeTestSkill(t, managed, "github", "github", "managed github") + + loader := NewLoader() + loader.ScanProjectSkills(project) + + assertSkillBody(t, loader, "custom", "project custom", "project") + assertSkillBody(t, loader, "github", "managed github", "managed") +} + +func TestManagedSkillsReserveExplicitSlashTriggers(t *testing.T) { + home := t.TempDir() + project := t.TempDir() + managed := filepath.Join(t.TempDir(), "managed") + t.Setenv("HOME", home) + t.Setenv(EnvManagedSkillsDir, managed) + t.Setenv(EnvReservedSkills, "github") + + writeTestSkill(t, managed, "provider-github", "github", "managed github") + writeTestSkillWithSlash( + t, + filepath.Join(home, ".jcode", "skills"), + "leading-slash-hijack", + "evil-leading", + "/github", + "user hijack", + ) + writeTestSkillWithSlash( + t, + filepath.Join(project, ".jcode", "skills"), + "bare-slash-hijack", + "evil-bare", + "github", + "project hijack", + ) + + loader := NewLoader() + loader.ScanProjectSkills(project) + + if got := loader.Get("evil-leading"); got != nil { + t.Fatalf("leading-slash hijack was loaded: %#v", got) + } + if got := loader.Get("evil-bare"); got != nil { + t.Fatalf("bare-slash hijack was loaded: %#v", got) + } + official := loader.GetBySlash("/github") + if official == nil || official.Name != "github" || official.Source != "managed" { + t.Fatalf("GetBySlash(/github) = %#v, want managed github", official) + } + if got := loader.GetBySlash("github"); got != nil { + t.Fatalf("bare slash lookup should not expose a second trigger: %#v", got) + } + + var githubCommands []*Skill + for _, skill := range loader.SlashCommands() { + if skill.Slash == "/github" { + githubCommands = append(githubCommands, skill) + } + } + if len(githubCommands) != 1 { + t.Fatalf("SlashCommands exposed %d /github commands: %#v", len(githubCommands), githubCommands) + } + if got := githubCommands[0]; got.Name != "github" || got.Source != "managed" { + t.Fatalf("SlashCommands /github = %#v, want managed github", got) + } +} + +func TestManagedSkillsRescanReappliesPriorityAndFailsClosed(t *testing.T) { + home := t.TempDir() + project := t.TempDir() + managedParent := t.TempDir() + managed := filepath.Join(managedParent, "managed") + t.Setenv("HOME", home) + t.Setenv(EnvManagedSkillsDir, managed) + t.Setenv(EnvReservedSkills, "github") + + writeTestSkill(t, filepath.Join(project, ".jcode", "skills"), "github", "github", "project github") + managedSkill := writeTestSkill(t, managed, "provider-github", "github", "managed v1") + + loader := NewLoader() + loader.ScanProjectSkills(project) + assertSkillBody(t, loader, "github", "managed v1", "managed") + + if err := os.WriteFile(managedSkill, []byte(skillMarkdown("github", "managed v2")), 0o600); err != nil { + t.Fatalf("update managed skill: %v", err) + } + loader.Rescan(project) + assertSkillBody(t, loader, "github", "managed v2", "managed") + + // A disappearing managed root must not reveal the project fallback. + if err := os.Rename(managed, managed+".offline"); err != nil { + t.Fatalf("make managed root unavailable: %v", err) + } + loader.Rescan(project) + if got := loader.Get("github"); got != nil { + t.Fatalf("reserved skill did not fail closed after rescan: %#v", got) + } +} + +func TestManagedSkillsRequireAbsoluteRootAndFailClosed(t *testing.T) { + home := t.TempDir() + project := t.TempDir() + t.Setenv("HOME", home) + t.Setenv(EnvManagedSkillsDir, "relative/managed") + t.Setenv(EnvReservedSkills, "github") + + writeTestSkill(t, filepath.Join(home, ".jcode", "skills"), "github", "github", "user github") + writeTestSkill(t, filepath.Join(project, ".jcode", "skills"), "github", "github", "project github") + + loader := NewLoader() + loader.ScanProjectSkills(project) + if got := loader.Get("github"); got != nil { + t.Fatalf("relative managed root exposed reserved fallback: %#v", got) + } +} + +func TestManagedSkillsEnvUnsetIsBackwardCompatible(t *testing.T) { + home := t.TempDir() + project := t.TempDir() + t.Setenv("HOME", home) + t.Setenv(EnvManagedSkillsDir, "") + t.Setenv(EnvReservedSkills, "") + + writeTestSkill(t, filepath.Join(home, ".agents", "skills"), "shared-agent", "shared", "agents") + writeTestSkill(t, filepath.Join(home, ".jcode", "skills"), "shared-user", "shared", "user") + writeTestSkill(t, filepath.Join(project, ".jcode", "skills"), "shared-project", "shared", "project") + + loader := NewLoader() + assertSkillBody(t, loader, "shared", "user", "user") + loader.ScanProjectSkills(project) + assertSkillBody(t, loader, "shared", "project", "project") + loader.Rescan(project) + assertSkillBody(t, loader, "shared", "project", "project") +} + +func writeTestSkill(t *testing.T, root, dirName, name, body string) string { + t.Helper() + dir := filepath.Join(root, dirName) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("create skill directory: %v", err) + } + path := filepath.Join(dir, "SKILL.md") + if err := os.WriteFile(path, []byte(skillMarkdown(name, body)), 0o600); err != nil { + t.Fatalf("write skill: %v", err) + } + return path +} + +func writeTestSkillWithSlash(t *testing.T, root, dirName, name, slash, body string) { + t.Helper() + dir := filepath.Join(root, dirName) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("create skill directory: %v", err) + } + path := filepath.Join(dir, "SKILL.md") + content := "---\nname: " + name + "\ndescription: " + body + "\nslash: " + slash + "\n---\n" + body + "\n" + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write skill: %v", err) + } +} + +func skillMarkdown(name, body string) string { + return "---\nname: " + name + "\ndescription: " + body + "\n---\n" + body + "\n" +} + +func assertSkillBody(t *testing.T, loader *Loader, name, body, source string) { + t.Helper() + skill := loader.Get(name) + if skill == nil { + t.Fatalf("skill %q was not loaded", name) + } + if skill.Body != body || skill.Source != source { + t.Fatalf("skill %q = body %q source %q, want body %q source %q", name, skill.Body, skill.Source, body, source) + } +} diff --git a/internal/skills/skills.go b/internal/skills/skills.go index d321a5f6..ac4b6b2f 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -21,14 +21,26 @@ type Skill struct { Body string // full markdown content (Layer 2, on-demand) Builtin bool // true if embedded in binary Path string // filesystem path (empty for built-in) - Source string // provenance: builtin | agents | user | project + Source string // provenance: builtin | agents | user | project | managed } +const ( + // EnvManagedSkillsDir points at a Cloud-managed skill root. The path must + // be absolute. Skills loaded from this root have the highest precedence. + EnvManagedSkillsDir = "JCODE_MANAGED_SKILLS_DIR" + // EnvReservedSkills is a comma/whitespace-separated list of skill names + // owned by the managed runtime. Lower-precedence sources cannot provide + // these names, even when the managed root is unavailable. + EnvReservedSkills = "JCODE_RESERVED_SKILLS" +) + // Loader discovers and caches skills from built-in embeds and user directories. type Loader struct { - mu sync.RWMutex - skills map[string]*Skill - disabled map[string]bool // skill names hidden from the agent + mu sync.RWMutex + skills map[string]*Skill + disabled map[string]bool // skill names hidden from the agent + managedSkillsDir string + reservedSkills map[string]bool } //go:embed builtin @@ -45,15 +57,18 @@ func NewLoader() *Loader { // commands, and the load_skill tool, but remain visible via All() for management UIs. func NewLoaderWithDisabled(disabled []string) *Loader { l := &Loader{ - skills: make(map[string]*Skill), - disabled: make(map[string]bool, len(disabled)), + skills: make(map[string]*Skill), + disabled: make(map[string]bool, len(disabled)), + managedSkillsDir: strings.TrimSpace(os.Getenv(EnvManagedSkillsDir)), + reservedSkills: parseReservedSkills(os.Getenv(EnvReservedSkills)), } for _, name := range disabled { l.disabled[name] = true } l.loadBuiltin() - l.ScanAgentsSkills() - l.ScanUserSkills() + l.scanAgentsSkills() + l.scanUserSkills() + l.refreshManagedSkills() return l } @@ -104,14 +119,19 @@ func (l *Loader) loadBuiltin() { // Each subdirectory (or symlink to a directory) containing a SKILL.md is treated as a skill. // User skills override built-in skills with the same name. func (l *Loader) ScanUserSkills() { - dir := filepath.Join(config.ConfigDir(), "skills") - l.scanDir(dir, "user") + l.scanUserSkills() + l.refreshManagedSkills() } // ScanAgentsSkills scans ~/.agents/skills/ for agent-defined skills. // Each subdirectory (or symlink to a directory) containing a SKILL.md is treated as a skill. // Agent skills are loaded before user skills, so user skills can override them. func (l *Loader) ScanAgentsSkills() { + l.scanAgentsSkills() + l.refreshManagedSkills() +} + +func (l *Loader) scanAgentsSkills() { homeDir, err := os.UserHomeDir() if err != nil { return @@ -120,10 +140,16 @@ func (l *Loader) ScanAgentsSkills() { l.scanDir(dir, "agents") } +func (l *Loader) scanUserSkills() { + dir := filepath.Join(config.ConfigDir(), "skills") + l.scanDir(dir, "user") +} + // ScanProjectSkills scans /.jcode/skills/ for project-local skills. func (l *Loader) ScanProjectSkills(projectDir string) { dir := filepath.Join(projectDir, ".jcode", "skills") l.scanDir(dir, "project") + l.refreshManagedSkills() } // scanDir scans a directory for skill subdirectories (including symlinks to directories). @@ -149,10 +175,94 @@ func (l *Loader) scanDir(dir, source string) { sk := parseSkill(skillName, string(data), false, fullPath) sk.Source = source l.mu.Lock() - l.skills[sk.Name] = sk + blocked := source != "managed" && l.conflictsWithReservedSkillLocked(sk) + if !blocked { + l.skills[sk.Name] = sk + } l.mu.Unlock() - config.Logger().Printf("[skills] loaded %s skill: %s from %s", source, sk.Name, sk.Path) + if blocked { + config.Logger().Printf( + "[skills] ignored %s skill %q from %s: name or slash trigger is reserved", + source, + sk.Name, + sk.Path, + ) + } else { + config.Logger().Printf("[skills] loaded %s skill: %s from %s", source, sk.Name, sk.Path) + } + } +} + +// refreshManagedSkills removes stale managed skills, hides all lower-priority +// definitions of reserved names, then loads the managed root last. Reserved +// names stay hidden when the root is missing or invalid (fail closed). +func (l *Loader) refreshManagedSkills() { + if l.managedSkillsDir == "" && len(l.reservedSkills) == 0 { + return + } + + l.mu.Lock() + for name, sk := range l.skills { + if sk.Source == "managed" || l.reservedSkills[name] { + delete(l.skills, name) + } + } + l.mu.Unlock() + + if l.managedSkillsDir == "" { + config.Logger().Printf( + "[skills] %s is required when %s is set; reserved skills remain hidden", + EnvManagedSkillsDir, + EnvReservedSkills, + ) + return + } + if !filepath.IsAbs(l.managedSkillsDir) { + config.Logger().Printf( + "[skills] %s must be an absolute path (got %q); reserved skills remain hidden", + EnvManagedSkillsDir, + l.managedSkillsDir, + ) + return + } + info, err := os.Stat(l.managedSkillsDir) + if err != nil { + config.Logger().Printf( + "[skills] managed skill root %q is unavailable: %v; reserved skills remain hidden", + l.managedSkillsDir, + err, + ) + return + } + if !info.IsDir() { + config.Logger().Printf( + "[skills] managed skill root %q is not a directory; reserved skills remain hidden", + l.managedSkillsDir, + ) + return + } + l.scanDir(l.managedSkillsDir, "managed") +} + +func (l *Loader) conflictsWithReservedSkillLocked(skill *Skill) bool { + if l.reservedSkills[skill.Name] { + return true + } + triggerName := strings.TrimPrefix(strings.TrimSpace(skill.Slash), "/") + return triggerName != "" && triggerName != "false" && l.reservedSkills[triggerName] +} + +func parseReservedSkills(raw string) map[string]bool { + fields := strings.FieldsFunc(raw, func(r rune) bool { + return r == ',' || r == ';' || r == '\n' || r == '\r' || r == '\t' || r == ' ' + }) + reserved := make(map[string]bool, len(fields)) + for _, name := range fields { + if name = strings.TrimSpace(name); name != "" { + reserved[name] = true + } } + return reserved } // Rescan re-scans all skill sources (preserving built-ins). @@ -165,11 +275,13 @@ func (l *Loader) Rescan(projectDir string) { } } l.mu.Unlock() - l.ScanAgentsSkills() - l.ScanUserSkills() + l.scanAgentsSkills() + l.scanUserSkills() if projectDir != "" { - l.ScanProjectSkills(projectDir) + dir := filepath.Join(projectDir, ".jcode", "skills") + l.scanDir(dir, "project") } + l.refreshManagedSkills() } // Get returns a skill by name, or nil if not found.