diff --git a/internal/gateway/methods/skills.go b/internal/gateway/methods/skills.go index 502d04a27..ac5349d12 100644 --- a/internal/gateway/methods/skills.go +++ b/internal/gateway/methods/skills.go @@ -10,6 +10,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/gateway" "github.com/nextlevelbuilder/goclaw/internal/i18n" "github.com/nextlevelbuilder/goclaw/internal/permissions" + "github.com/nextlevelbuilder/goclaw/internal/skills" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/pkg/protocol" ) @@ -38,6 +39,12 @@ func (m *SkillsMethods) Register(router *gateway.MethodRouter) { func (m *SkillsMethods) handleList(ctx context.Context, client *gateway.Client, req *protocol.RequestFrame) { allSkills := m.store.ListSkills(ctx) + // Visibility filter: non-admins see system skills, public skills, and + // their own private skills. Admins see everything in the tenant. + if !permissions.HasMinRole(client.Role(), permissions.RoleAdmin) { + allSkills = store.FilterVisibleSkills(ctx, allSkills) + } + result := make([]map[string]any, 0, len(allSkills)) for _, s := range allSkills { entry := map[string]any{ @@ -116,6 +123,13 @@ func (m *SkillsMethods) handleGet(ctx context.Context, client *gateway.Client, r return } + // Visibility gate: hide private skills from non-owners (admins bypass). + if !permissions.HasMinRole(client.Role(), permissions.RoleAdmin) && + !store.IsSkillVisibleTo(ctx, info.OwnerID, info.Visibility, info.IsSystem) { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrNotFound, i18n.T(locale, i18n.MsgNotFound, "skill", params.Name))) + return + } + content, _ := m.store.LoadSkill(ctx, params.Name) resp := map[string]any{ @@ -196,8 +210,9 @@ func (m *SkillsMethods) handleUpdate(ctx context.Context, client *gateway.Client return } - // Ownership check: only skill owner or admin can update. + // Ownership check first: only skill owner or admin can update. // Fail-closed: if store doesn't implement skillOwnerGetter, deny non-admin callers. + // Auth-before-validate avoids leaking skill-existence info via validation errors. if !permissions.HasMinRole(client.Role(), permissions.RoleAdmin) { ownerGetter, ok := m.store.(skillOwnerGetter) if !ok { @@ -210,6 +225,18 @@ func (m *SkillsMethods) handleUpdate(ctx context.Context, client *gateway.Client } } + // Validate visibility enum if present — fail closed before mutating the DB. + if v, ok := params.Updates["visibility"]; ok { + vs, _ := v.(string) + if err := skills.ValidateVisibility(vs); err != nil { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgInvalidVisibility, vs))) + return + } + if vs != "" { + params.Updates["visibility"] = skills.NormalizeVisibility(vs) + } + } + if err := updater.UpdateSkill(ctx, skillID, params.Updates); err != nil { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInternal, err.Error())) return diff --git a/internal/i18n/catalog_en.go b/internal/i18n/catalog_en.go index 61af216af..681771adc 100644 --- a/internal/i18n/catalog_en.go +++ b/internal/i18n/catalog_en.go @@ -113,6 +113,7 @@ func init() { // Skills MsgSkillsUpdateNotSupported: "skills.update not supported for file-based skills", MsgCannotResolveSkillID: "cannot resolve skill ID for file-based skill", + MsgInvalidVisibility: "invalid visibility %q: must be one of private, public", // Logs MsgInvalidLogAction: "action must be 'start' or 'stop'", diff --git a/internal/i18n/catalog_vi.go b/internal/i18n/catalog_vi.go index 93ba0d973..af6fc6adf 100644 --- a/internal/i18n/catalog_vi.go +++ b/internal/i18n/catalog_vi.go @@ -113,6 +113,7 @@ func init() { // Skills MsgSkillsUpdateNotSupported: "skills.update không được hỗ trợ với skill dựa trên tệp", MsgCannotResolveSkillID: "không thể xác định ID skill dựa trên tệp", + MsgInvalidVisibility: "visibility không hợp lệ %q: phải là private hoặc public", // Logs MsgInvalidLogAction: "action phải là 'start' hoặc 'stop'", diff --git a/internal/i18n/catalog_zh.go b/internal/i18n/catalog_zh.go index 0d840cdb7..ea5c3cdea 100644 --- a/internal/i18n/catalog_zh.go +++ b/internal/i18n/catalog_zh.go @@ -113,6 +113,7 @@ func init() { // Skills MsgSkillsUpdateNotSupported: "基于文件的Skill不支持 skills.update", MsgCannotResolveSkillID: "无法解析基于文件的Skill ID", + MsgInvalidVisibility: "无效的 visibility %q：必须为 private 或 public", // Logs MsgInvalidLogAction: "action 必须是 'start' 或 'stop'", diff --git a/internal/i18n/keys.go b/internal/i18n/keys.go index 348012ff3..17a40b164 100644 --- a/internal/i18n/keys.go +++ b/internal/i18n/keys.go @@ -114,6 +114,7 @@ const ( // --- Skills --- MsgSkillsUpdateNotSupported = "error.skills_update_not_supported" // "skills.update not supported for file-based skills" MsgCannotResolveSkillID = "error.cannot_resolve_skill_id" // "cannot resolve skill ID for file-based skill" + MsgInvalidVisibility = "error.invalid_visibility" // "invalid visibility %q: must be one of private, public" // --- Logs --- MsgInvalidLogAction = "error.invalid_log_action" // "action must be 'start' or 'stop'" diff --git a/internal/skills/visibility.go b/internal/skills/visibility.go new file mode 100644 index 000000000..c923ed71a --- /dev/null +++ b/internal/skills/visibility.go @@ -0,0 +1,51 @@ +package skills + +import ( + "fmt" + "strings" +) + +// Skill visibility values. +const ( + VisibilityPrivate = "private" + VisibilityPublic = "public" +) + +// DefaultVisibility is assigned when a caller does not specify one. +// Private matches the historical hardcoded default and is the safer choice. +const DefaultVisibility = VisibilityPrivate + +// validVisibilities enumerates the accepted enum values. System skills use +// "public"; user-published skills default to "private". +var validVisibilities = map[string]struct{}{ + VisibilityPrivate: {}, + VisibilityPublic: {}, +} + +// NormalizeVisibility lowercases + trims the input and returns the default +// when empty. It does not validate — pair with ValidateVisibility. +func NormalizeVisibility(v string) string { + v = strings.ToLower(strings.TrimSpace(v)) + if v == "" { + return DefaultVisibility + } + return v +} + +// ValidateVisibility returns an error if v is not one of the supported enum +// values. An empty string is treated as valid (caller applies the default). +func ValidateVisibility(v string) error { + if v == "" { + return nil + } + if _, ok := validVisibilities[strings.ToLower(strings.TrimSpace(v))]; !ok { + return fmt.Errorf("invalid visibility %q: must be one of private, public", v) + } + return nil +} + +// IsValidVisibility reports whether v is a recognized enum value. Empty is false. +func IsValidVisibility(v string) bool { + _, ok := validVisibilities[strings.ToLower(strings.TrimSpace(v))] + return ok +} diff --git a/internal/skills/visibility_test.go b/internal/skills/visibility_test.go new file mode 100644 index 000000000..06d7ff437 --- /dev/null +++ b/internal/skills/visibility_test.go @@ -0,0 +1,41 @@ +package skills + +import "testing" + +func TestValidateVisibility(t *testing.T) { + tests := []struct { + name string + input string + wantErr bool + }{ + {"empty ok (caller defaults)", "", false}, + {"private", "private", false}, + {"public", "public", false}, + {"uppercase normalized", "PRIVATE", false}, + {"whitespace normalized", " public ", false}, + {"team rejected (v1 scope)", "team", true}, + {"garbage rejected", "nope", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateVisibility(tt.input) + if (err != nil) != tt.wantErr { + t.Fatalf("ValidateVisibility(%q) err=%v, wantErr=%v", tt.input, err, tt.wantErr) + } + }) + } +} + +func TestNormalizeVisibility(t *testing.T) { + cases := map[string]string{ + "": DefaultVisibility, + "private": "private", + "PUBLIC": "public", + " public ": "public", + } + for in, want := range cases { + if got := NormalizeVisibility(in); got != want { + t.Errorf("NormalizeVisibility(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/store/pg/skills.go b/internal/store/pg/skills.go index d40e822c6..e8f35eb82 100644 --- a/internal/store/pg/skills.go +++ b/internal/store/pg/skills.go @@ -78,7 +78,7 @@ func (s *PGSkillStore) ListSkills(ctx context.Context) []store.SkillInfo { // Tenant filter: system skills visible globally, custom skills scoped to tenant. var scanned []skillInfoRowWithFrontmatter if err := pkgSqlxDB.SelectContext(ctx, &scanned, - `SELECT id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, frontmatter, file_path + `SELECT id, name, slug, description, visibility, owner_id, tags, version, is_system, status, enabled, deps, frontmatter, file_path FROM skills WHERE (status IN ('active', 'archived') OR is_system = true) AND (is_system = true OR tenant_id = $1) ORDER BY name`, tid); err != nil { return nil @@ -105,7 +105,7 @@ func (s *PGSkillStore) ListAllSkills(ctx context.Context) []store.SkillInfo { } var scanned []skillInfoRow if err := pkgSqlxDB.SelectContext(ctx, &scanned, - `SELECT id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, file_path + `SELECT id, name, slug, description, visibility, owner_id, tags, version, is_system, status, enabled, deps, file_path FROM skills WHERE enabled = true AND status != 'deleted' AND (is_system = true OR tenant_id = $1) ORDER BY name`, tid); err != nil { return nil @@ -118,7 +118,7 @@ func (s *PGSkillStore) ListAllSkills(ctx context.Context) []store.SkillInfo { func (s *PGSkillStore) ListAllSystemSkills(ctx context.Context) []store.SkillInfo { var scanned []skillInfoRow if err := pkgSqlxDB.SelectContext(ctx, &scanned, - `SELECT id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, file_path + `SELECT id, name, slug, description, visibility, owner_id, tags, version, is_system, status, enabled, deps, file_path FROM skills WHERE is_system = true AND enabled = true AND status != 'deleted' ORDER BY name`); err != nil { return nil diff --git a/internal/store/pg/skills_content.go b/internal/store/pg/skills_content.go index 6e157176a..1ac30c411 100644 --- a/internal/store/pg/skills_content.go +++ b/internal/store/pg/skills_content.go @@ -90,13 +90,13 @@ func (s *PGSkillStore) BuildSummary(ctx context.Context, allowList []string) str func (s *PGSkillStore) GetSkill(ctx context.Context, name string) (*store.SkillInfo, bool) { var id uuid.UUID - var skillName, slug, visibility string + var skillName, slug, visibility, ownerID string var desc *string var tags []string var version int var isSystem bool var filePath *string - q := "SELECT id, name, slug, description, visibility, tags, version, is_system, file_path FROM skills WHERE slug = $1 AND status = 'active'" + q := "SELECT id, name, slug, description, visibility, owner_id, tags, version, is_system, file_path FROM skills WHERE slug = $1 AND status = 'active'" args := []any{name} if !store.IsCrossTenant(ctx) { tid := store.TenantIDFromContext(ctx) @@ -106,12 +106,13 @@ func (s *PGSkillStore) GetSkill(ctx context.Context, name string) (*store.SkillI q += " AND (is_system = true OR tenant_id = $2)" args = append(args, tid) } - err := s.db.QueryRowContext(ctx, q, args...).Scan(&id, &skillName, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem, &filePath) + err := s.db.QueryRowContext(ctx, q, args...).Scan(&id, &skillName, &slug, &desc, &visibility, &ownerID, pq.Array(&tags), &version, &isSystem, &filePath) if err != nil { return nil, false } info := buildSkillInfo(id.String(), skillName, slug, desc, version, s.baseDir, filePath) info.Visibility = visibility + info.OwnerID = ownerID info.Tags = tags info.IsSystem = isSystem return &info, true @@ -121,9 +122,11 @@ func (s *PGSkillStore) FilterSkills(ctx context.Context, allowList []string) []s all := s.ListSkills(ctx) var filtered []store.SkillInfo if allowList == nil { - // No allowList → return all enabled skills (for agent injection) + // No allowList → return all enabled skills visible to the caller + // (for agent injection). Private skills owned by others are hidden + // so they don't leak across tenant members. for _, sk := range all { - if sk.Enabled { + if sk.Enabled && store.IsSkillVisibleTo(ctx, sk.OwnerID, sk.Visibility, sk.IsSystem) { filtered = append(filtered, sk) } } @@ -148,14 +151,14 @@ func (s *PGSkillStore) FilterSkills(ctx context.Context, allowList []string) []s // Used by admin operations (e.g. toggle) that need full skill info. // Tenant filter: system skills visible globally, custom skills scoped to tenant. func (s *PGSkillStore) GetSkillByID(ctx context.Context, id uuid.UUID) (store.SkillInfo, bool) { - var name, slug, visibility, status string + var name, slug, visibility, ownerID, status string var desc *string var tags []string var version int var isSystem, enabled bool var depsRaw []byte var filePath *string - q := `SELECT name, slug, description, visibility, tags, version, is_system, status, enabled, deps, file_path + q := `SELECT name, slug, description, visibility, owner_id, tags, version, is_system, status, enabled, deps, file_path FROM skills WHERE id = $1` args := []any{id} if !store.IsCrossTenant(ctx) { @@ -166,12 +169,13 @@ func (s *PGSkillStore) GetSkillByID(ctx context.Context, id uuid.UUID) (store.Sk q += " AND (is_system = true OR tenant_id = $2)" args = append(args, tid) } - err := s.db.QueryRowContext(ctx, q, args...).Scan(&name, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem, &status, &enabled, &depsRaw, &filePath) + err := s.db.QueryRowContext(ctx, q, args...).Scan(&name, &slug, &desc, &visibility, &ownerID, pq.Array(&tags), &version, &isSystem, &status, &enabled, &depsRaw, &filePath) if err != nil { return store.SkillInfo{}, false } info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir, filePath) info.Visibility = visibility + info.OwnerID = ownerID info.Tags = tags info.IsSystem = isSystem info.Status = status diff --git a/internal/store/pg/skills_scan_rows.go b/internal/store/pg/skills_scan_rows.go index 9a1df82d3..a7a568e87 100644 --- a/internal/store/pg/skills_scan_rows.go +++ b/internal/store/pg/skills_scan_rows.go @@ -18,6 +18,7 @@ type skillInfoRow struct { Slug string `db:"slug"` Desc *string `db:"description"` Visibility string `db:"visibility"` + OwnerID string `db:"owner_id"` Tags pq.StringArray `db:"tags"` Version int `db:"version"` IsSystem bool `db:"is_system"` @@ -37,6 +38,7 @@ type skillInfoRowWithFrontmatter struct { func (r *skillInfoRow) toSkillInfo(baseDir string) store.SkillInfo { info := buildSkillInfo(r.ID.String(), r.Name, r.Slug, r.Desc, r.Version, baseDir, r.FilePath) info.Visibility = r.Visibility + info.OwnerID = r.OwnerID info.Tags = []string(r.Tags) info.IsSystem = r.IsSystem info.Status = r.Status diff --git a/internal/store/skill_store.go b/internal/store/skill_store.go index b5b18f391..478725eec 100644 --- a/internal/store/skill_store.go +++ b/internal/store/skill_store.go @@ -16,6 +16,7 @@ type SkillInfo struct { Source string `json:"source" db:"-"` Description string `json:"description" db:"description"` Visibility string `json:"visibility,omitempty" db:"visibility"` + OwnerID string `json:"owner_id,omitempty" db:"owner_id"` Tags []string `json:"tags,omitempty" db:"tags"` Version int `json:"version,omitempty" db:"version"` IsSystem bool `json:"is_system,omitempty" db:"is_system"` diff --git a/internal/store/sqlitestore/skills.go b/internal/store/sqlitestore/skills.go index f2cc1bfe0..9d41ae433 100644 --- a/internal/store/sqlitestore/skills.go +++ b/internal/store/sqlitestore/skills.go @@ -71,7 +71,7 @@ func (s *SQLiteSkillStore) ListSkills(ctx context.Context) []store.SkillInfo { s.mu.RUnlock() rows, err := s.db.QueryContext(ctx, - `SELECT id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, frontmatter, file_path + `SELECT id, name, slug, description, visibility, owner_id, tags, version, is_system, status, enabled, deps, frontmatter, file_path FROM skills WHERE (status IN ('active', 'archived') OR is_system = 1) AND (is_system = 1 OR tenant_id = ?) ORDER BY name`, tid) if err != nil { @@ -82,19 +82,20 @@ func (s *SQLiteSkillStore) ListSkills(ctx context.Context) []store.SkillInfo { var result []store.SkillInfo for rows.Next() { var id uuid.UUID - var name, slug, visibility, status string + var name, slug, visibility, ownerID, status string var desc *string var tagsJSON []byte var version int var isSystem, enabled bool var depsRaw, fmRaw []byte var filePath *string - if err := rows.Scan(&id, &name, &slug, &desc, &visibility, &tagsJSON, &version, + if err := rows.Scan(&id, &name, &slug, &desc, &visibility, &ownerID, &tagsJSON, &version, &isSystem, &status, &enabled, &depsRaw, &fmRaw, &filePath); err != nil { continue } info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir, filePath) info.Visibility = visibility + info.OwnerID = ownerID scanJSONStringArray(tagsJSON, &info.Tags) info.IsSystem = isSystem info.Status = status diff --git a/internal/store/sqlitestore/skills_content.go b/internal/store/sqlitestore/skills_content.go index 8fc7df918..ad27c604a 100644 --- a/internal/store/sqlitestore/skills_content.go +++ b/internal/store/sqlitestore/skills_content.go @@ -85,13 +85,13 @@ func (s *SQLiteSkillStore) BuildSummary(ctx context.Context, allowList []string) func (s *SQLiteSkillStore) GetSkill(ctx context.Context, name string) (*store.SkillInfo, bool) { var id uuid.UUID - var skillName, slug, visibility string + var skillName, slug, visibility, ownerID string var desc *string var tagsJSON []byte var version int var isSystem bool var filePath *string - q := "SELECT id, name, slug, description, visibility, tags, version, is_system, file_path FROM skills WHERE slug = ? AND status = 'active'" + q := "SELECT id, name, slug, description, visibility, owner_id, tags, version, is_system, file_path FROM skills WHERE slug = ? AND status = 'active'" args := []any{name} if !store.IsCrossTenant(ctx) { tid := store.TenantIDFromContext(ctx) @@ -101,11 +101,12 @@ func (s *SQLiteSkillStore) GetSkill(ctx context.Context, name string) (*store.Sk q += " AND (is_system = 1 OR tenant_id = ?)" args = append(args, tid) } - if err := s.db.QueryRowContext(ctx, q, args...).Scan(&id, &skillName, &slug, &desc, &visibility, &tagsJSON, &version, &isSystem, &filePath); err != nil { + if err := s.db.QueryRowContext(ctx, q, args...).Scan(&id, &skillName, &slug, &desc, &visibility, &ownerID, &tagsJSON, &version, &isSystem, &filePath); err != nil { return nil, false } info := buildSkillInfo(id.String(), skillName, slug, desc, version, s.baseDir, filePath) info.Visibility = visibility + info.OwnerID = ownerID scanJSONStringArray(tagsJSON, &info.Tags) info.IsSystem = isSystem return &info, true @@ -116,7 +117,7 @@ func (s *SQLiteSkillStore) FilterSkills(ctx context.Context, allowList []string) var filtered []store.SkillInfo if allowList == nil { for _, sk := range all { - if sk.Enabled { + if sk.Enabled && store.IsSkillVisibleTo(ctx, sk.OwnerID, sk.Visibility, sk.IsSystem) { filtered = append(filtered, sk) } } @@ -139,13 +140,13 @@ func (s *SQLiteSkillStore) FilterSkills(ctx context.Context, allowList []string) // GetSkillByID returns a SkillInfo for any skill by UUID regardless of status. func (s *SQLiteSkillStore) GetSkillByID(ctx context.Context, id uuid.UUID) (store.SkillInfo, bool) { - var name, slug, visibility, status string + var name, slug, visibility, ownerID, status string var desc *string var tagsJSON, depsRaw []byte var version int var isSystem, enabled bool var filePath *string - q := `SELECT name, slug, description, visibility, tags, version, is_system, status, enabled, deps, file_path + q := `SELECT name, slug, description, visibility, owner_id, tags, version, is_system, status, enabled, deps, file_path FROM skills WHERE id = ?` args := []any{id} if !store.IsCrossTenant(ctx) { @@ -156,12 +157,13 @@ func (s *SQLiteSkillStore) GetSkillByID(ctx context.Context, id uuid.UUID) (stor q += " AND (is_system = 1 OR tenant_id = ?)" args = append(args, tid) } - if err := s.db.QueryRowContext(ctx, q, args...).Scan(&name, &slug, &desc, &visibility, &tagsJSON, + if err := s.db.QueryRowContext(ctx, q, args...).Scan(&name, &slug, &desc, &visibility, &ownerID, &tagsJSON, &version, &isSystem, &status, &enabled, &depsRaw, &filePath); err != nil { return store.SkillInfo{}, false } info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir, filePath) info.Visibility = visibility + info.OwnerID = ownerID scanJSONStringArray(tagsJSON, &info.Tags) info.IsSystem = isSystem info.Status = status diff --git a/internal/store/visibility_filter.go b/internal/store/visibility_filter.go new file mode 100644 index 000000000..b08ec11e3 --- /dev/null +++ b/internal/store/visibility_filter.go @@ -0,0 +1,53 @@ +package store + +import ( + "context" + "strings" +) + +// IsSkillVisibleTo returns true if the caller identified by ctx can discover +// the given skill. Rules: +// - System skills are visible to everyone. +// - Empty or "public" visibility is treated as public (legacy rows default +// to "public" for safety since older stores did not enforce the field). +// - "private" skills are only visible to the owner. Three identity strings +// are considered (actor, user, sender) to match the same identities +// isOwnerOfSkill checks for backward compatibility (#915). +// +// Admin/master-scope bypass is the caller's responsibility — this helper +// reflects the non-privileged baseline. +func IsSkillVisibleTo(ctx context.Context, ownerID, visibility string, isSystem bool) bool { + if isSystem { + return true + } + // Normalize to defend against historical rows with mixed case / whitespace + // that bypassed the write-path normalizer. + switch strings.ToLower(strings.TrimSpace(visibility)) { + case "", "public": + return true + case "private": + if ownerID == "" { + // No owner recorded — treat as public (historical data). + return true + } + actorID := ActorIDFromContext(ctx) + userID := UserIDFromContext(ctx) + senderID := SenderIDFromContext(ctx) + return ownerID == actorID || ownerID == userID || ownerID == senderID + default: + // Unknown enum value: fail closed (hide). + return false + } +} + +// FilterVisibleSkills returns skills the caller can discover. Uses +// IsSkillVisibleTo for each entry. +func FilterVisibleSkills(ctx context.Context, skills []SkillInfo) []SkillInfo { + out := make([]SkillInfo, 0, len(skills)) + for _, s := range skills { + if IsSkillVisibleTo(ctx, s.OwnerID, s.Visibility, s.IsSystem) { + out = append(out, s) + } + } + return out +} diff --git a/internal/store/visibility_filter_test.go b/internal/store/visibility_filter_test.go new file mode 100644 index 000000000..ddd6419f6 --- /dev/null +++ b/internal/store/visibility_filter_test.go @@ -0,0 +1,62 @@ +package store + +import ( + "context" + "testing" +) + +func TestIsSkillVisibleTo(t *testing.T) { + alice := "alice" + bob := "bob" + ctx := WithUserID(context.Background(), alice) + + tests := []struct { + name string + owner string + visibility string + isSystem bool + want bool + }{ + {"system skill visible to anyone", "system", "private", true, true}, + {"public visible to non-owner", bob, "public", false, true}, + {"empty visibility treated as public", bob, "", false, true}, + {"private visible to owner", alice, "private", false, true}, + {"private hidden from non-owner", bob, "private", false, false}, + {"private with no owner treated as public", "", "private", false, true}, + {"unknown enum fails closed", bob, "team", false, false}, + {"uppercase private matched for owner", alice, "PRIVATE", false, true}, + {"whitespace public treated as public", bob, " public ", false, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsSkillVisibleTo(ctx, tt.owner, tt.visibility, tt.isSystem) + if got != tt.want { + t.Fatalf("IsSkillVisibleTo(owner=%q, vis=%q, sys=%v) = %v, want %v", + tt.owner, tt.visibility, tt.isSystem, got, tt.want) + } + }) + } +} + +func TestFilterVisibleSkills(t *testing.T) { + ctx := WithUserID(context.Background(), "alice") + skills := []SkillInfo{ + {Slug: "sys", IsSystem: true, Visibility: "public"}, + {Slug: "mine-private", OwnerID: "alice", Visibility: "private"}, + {Slug: "theirs-private", OwnerID: "bob", Visibility: "private"}, + {Slug: "theirs-public", OwnerID: "bob", Visibility: "public"}, + } + got := FilterVisibleSkills(ctx, skills) + gotSlugs := map[string]bool{} + for _, s := range got { + gotSlugs[s.Slug] = true + } + for _, want := range []string{"sys", "mine-private", "theirs-public"} { + if !gotSlugs[want] { + t.Errorf("expected %q in filtered output, got %v", want, gotSlugs) + } + } + if gotSlugs["theirs-private"] { + t.Errorf("leaked private skill to non-owner: %v", gotSlugs) + } +} diff --git a/internal/tools/publish_skill.go b/internal/tools/publish_skill.go index 573354790..dd3f50a76 100644 --- a/internal/tools/publish_skill.go +++ b/internal/tools/publish_skill.go @@ -56,6 +56,11 @@ func (t *PublishSkillTool) Parameters() map[string]any { "type": "string", "description": "Path to skill directory containing SKILL.md (absolute or relative to workspace)", }, + "visibility": map[string]any{ + "type": "string", + "enum": []string{skills.VisibilityPrivate, skills.VisibilityPublic}, + "description": "Who can discover this skill. 'private' (default) is visible only to the owner; 'public' is visible to anyone in the tenant.", + }, }, "required": []string{"path"}, } @@ -67,6 +72,12 @@ func (t *PublishSkillTool) Execute(ctx context.Context, args map[string]any) *Re return ErrorResult("path is required") } + rawVisibility, _ := args["visibility"].(string) + if err := skills.ValidateVisibility(rawVisibility); err != nil { + return ErrorResult(err.Error()) + } + visibility := skills.NormalizeVisibility(rawVisibility) + // Resolve path: absolute or relative to workspace dir := rawPath if !filepath.IsAbs(dir) { @@ -141,7 +152,7 @@ func (t *PublishSkillTool) Execute(ctx context.Context, args map[string]any) *Re Slug: slug, Description: &desc, OwnerID: ownerID, - Visibility: "private", + Visibility: visibility, Version: version, FilePath: destDir, FileSize: fileSize, diff --git a/internal/tools/skill_manage.go b/internal/tools/skill_manage.go index 7ea13e1e7..16b70545d 100644 --- a/internal/tools/skill_manage.go +++ b/internal/tools/skill_manage.go @@ -87,12 +87,17 @@ func (t *SkillManageTool) Parameters() map[string]any { }, "find": map[string]any{ "type": "string", - "description": "Exact text to find in the current SKILL.md. Required for patch.", + "description": "Exact text to find in the current SKILL.md. Required for patch unless only 'visibility' is being updated.", }, "replace": map[string]any{ "type": "string", "description": "Replacement text. Required for patch.", }, + "visibility": map[string]any{ + "type": "string", + "enum": []string{skills.VisibilityPrivate, skills.VisibilityPublic}, + "description": "Skill visibility. For create: defaults to 'private'. For patch: updates who can discover the skill without creating a new version.", + }, }, "required": []string{"action"}, } @@ -125,6 +130,12 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any return ErrorResult(fmt.Sprintf("content too large (%d bytes, max %d)", len(content), maxSkillContentSize)) } + rawVisibility, _ := args["visibility"].(string) + if err := skills.ValidateVisibility(rawVisibility); err != nil { + return ErrorResult(err.Error()) + } + visibility := skills.NormalizeVisibility(rawVisibility) + // Security scan before any disk write violations, safe := skills.GuardSkillContent(content) if !safe { @@ -183,7 +194,7 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any Slug: slug, Description: &desc, OwnerID: ownerID, - Visibility: "private", + Visibility: visibility, Version: version, FilePath: destDir, FileSize: fileSize, @@ -238,11 +249,16 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) slug, _ := args["slug"].(string) find, _ := args["find"].(string) replace, _ := args["replace"].(string) + rawVisibility, _ := args["visibility"].(string) if slug == "" { return ErrorResult("slug is required for action=patch") } - if find == "" { - return ErrorResult("find is required for action=patch") + if err := skills.ValidateVisibility(rawVisibility); err != nil { + return ErrorResult(err.Error()) + } + // Patch requires at least one of: content edit (find) or visibility change. + if find == "" && rawVisibility == "" { + return ErrorResult("patch requires either 'find' (content edit) or 'visibility' (metadata update)") } info, ok := t.skills.GetSkill(ctx, slug) @@ -266,6 +282,26 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug)) } + // Visibility-only patch path: no content change, no new version. + if find == "" && rawVisibility != "" { + skillID, err := uuid.Parse(info.ID) + if err != nil { + return ErrorResult(fmt.Sprintf("invalid skill ID in database: %v", err)) + } + newVisibility := skills.NormalizeVisibility(rawVisibility) + if err := t.skills.UpdateSkill(ctx, skillID, map[string]any{ + "visibility": newVisibility, + "updated_at": time.Now(), + }); err != nil { + return ErrorResult(fmt.Sprintf("failed to update skill visibility: %v", err)) + } + slog.Info("skill_manage: visibility updated", "slug", slug, "visibility", newVisibility) + if t.loader != nil { + t.loader.BumpVersion() + } + return NewResult(fmt.Sprintf("Skill %q visibility set to %s.", slug, newVisibility)) + } + // Read current SKILL.md from latest version current, err := os.ReadFile(info.Path) if err != nil { @@ -316,13 +352,17 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) if err != nil { return ErrorResult(fmt.Sprintf("invalid skill ID in database: %v", err)) } - if err := t.skills.UpdateSkill(ctx, skillID, map[string]any{ + updates := map[string]any{ "version": newVer, "file_path": destDir, "file_size": fileSize, "file_hash": &fileHash, "updated_at": time.Now(), - }); err != nil { + } + if rawVisibility != "" { + updates["visibility"] = skills.NormalizeVisibility(rawVisibility) + } + if err := t.skills.UpdateSkill(ctx, skillID, updates); err != nil { return ErrorResult(fmt.Sprintf("failed to update skill in database: %v", err)) }