From 0bf7f88054f091532e0fbb4ded94a7a82779e97d Mon Sep 17 00:00:00 2001 From: Goon Date: Fri, 12 Jun 2026 10:38:53 +0700 Subject: [PATCH] feat(skills): add lifecycle API and CLI --- cmd/gateway_http_client.go | 4 + cmd/skills_cmd.go | 4 + cmd/skills_lifecycle_cmd.go | 166 +++++++ cmd/skills_lifecycle_cmd_test.go | 159 +++++++ cmd/skills_lifecycle_helpers.go | 134 ++++++ ...21-agent-evolution-and-skill-management.md | 12 +- docs/project-changelog.md | 18 + internal/http/skills.go | 32 +- internal/http/skills_access.go | 152 ++++++ internal/http/skills_access_effective.go | 111 +++++ internal/http/skills_grants.go | 59 ++- internal/http/skills_lifecycle.go | 144 ++++++ internal/http/skills_lifecycle_deps.go | 164 +++++++ internal/http/skills_lifecycle_test.go | 437 ++++++++++++++++++ internal/http/skills_upload_test.go | 116 ++++- internal/skills/dep_installer.go | 1 + internal/skills/dep_scanner.go | 2 +- internal/store/pg/skills_grants.go | 71 ++- internal/store/skill_store.go | 7 + internal/store/sqlitestore/schema.go | 19 +- internal/store/sqlitestore/schema.sql | 2 +- internal/store/sqlitestore/skills.go | 23 +- internal/store/sqlitestore/skills_grants.go | 91 +++- internal/store/sqlitestore/skills_test.go | 107 +++++ internal/tools/skill_manage_files_test.go | 3 + internal/upgrade/version.go | 2 +- ...8_skill_user_grants_tenant_unique.down.sql | 6 + ...078_skill_user_grants_tenant_unique.up.sql | 9 + tests/integration/v3_skills_store_test.go | 75 +++ 29 files changed, 2065 insertions(+), 65 deletions(-) create mode 100644 cmd/skills_lifecycle_cmd.go create mode 100644 cmd/skills_lifecycle_cmd_test.go create mode 100644 cmd/skills_lifecycle_helpers.go create mode 100644 internal/http/skills_access.go create mode 100644 internal/http/skills_access_effective.go create mode 100644 internal/http/skills_lifecycle.go create mode 100644 internal/http/skills_lifecycle_deps.go create mode 100644 internal/http/skills_lifecycle_test.go create mode 100644 migrations/000078_skill_user_grants_tenant_unique.down.sql create mode 100644 migrations/000078_skill_user_grants_tenant_unique.up.sql diff --git a/cmd/gateway_http_client.go b/cmd/gateway_http_client.go index b1788a67..179c2bdc 100644 --- a/cmd/gateway_http_client.go +++ b/cmd/gateway_http_client.go @@ -95,6 +95,10 @@ func gatewayHTTPPut(path string, body any) (map[string]any, error) { return gatewayHTTPDo(http.MethodPut, path, body) } +func gatewayHTTPPatch(path string, body any) (map[string]any, error) { + return gatewayHTTPDo(http.MethodPatch, path, body) +} + func gatewayHTTPDelete(path string) error { _, err := gatewayHTTPDo(http.MethodDelete, path, nil) return err diff --git a/cmd/skills_cmd.go b/cmd/skills_cmd.go index d87e1706..d34285af 100644 --- a/cmd/skills_cmd.go +++ b/cmd/skills_cmd.go @@ -22,6 +22,10 @@ func skillsCmd() *cobra.Command { } cmd.AddCommand(skillsListCmd()) cmd.AddCommand(skillsShowCmd()) + cmd.AddCommand(skillsDepsCmd()) + cmd.AddCommand(skillsAccessCmd()) + cmd.AddCommand(skillsGrantCmd()) + cmd.AddCommand(skillsRevokeCmd()) return cmd } diff --git a/cmd/skills_lifecycle_cmd.go b/cmd/skills_lifecycle_cmd.go new file mode 100644 index 00000000..bf9fc27c --- /dev/null +++ b/cmd/skills_lifecycle_cmd.go @@ -0,0 +1,166 @@ +package cmd + +import ( + "fmt" + "net/http" + "net/url" + + "github.com/spf13/cobra" +) + +var ( + skillsGatewayDo = gatewayHTTPDo + skillsGatewayDelete = gatewayHTTPDelete + skillsRequireGateway = requireRunningGatewayHTTP +) + +func skillsDepsCmd() *cobra.Command { + cmd := &cobra.Command{Use: "deps", Short: "Scan, check, and install skill dependencies"} + cmd.AddCommand(skillsDepsReadCmd("status", http.MethodGet, "Show dependency status")) + cmd.AddCommand(skillsDepsReadCmd("scan", http.MethodPost, "Scan dependency declarations")) + cmd.AddCommand(skillsDepsReadCmd("check", http.MethodPost, "Check dependency availability")) + cmd.AddCommand(skillsDepsInstallCmd()) + return cmd +} + +func skillsDepsReadCmd(name, method, short string) *cobra.Command { + var jsonOutput bool + cmd := &cobra.Command{ + Use: name + " [skill-id-or-path]", + Short: short, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if pathExists(args[0]) { + return runLocalSkillDepsStatus(cmd.OutOrStdout(), args[0], jsonOutput) + } + skillsRequireGateway() + path := "/v1/skills/" + url.PathEscape(args[0]) + "/dependencies" + if method == http.MethodPost { + path += "/" + name + } + return runSkillsGateway(cmd.OutOrStdout(), method, path, nil, jsonOutput) + }, + } + cmd.Flags().BoolVar(&jsonOutput, "json", false, "output as JSON") + return cmd +} + +func skillsDepsInstallCmd() *cobra.Command { + var jsonOutput bool + cmd := &cobra.Command{ + Use: "install [skill-id]", + Short: "Install missing dependencies for a managed skill", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + skillsRequireGateway() + path := "/v1/skills/" + url.PathEscape(args[0]) + "/dependencies/install" + return runSkillsGateway(cmd.OutOrStdout(), http.MethodPost, path, nil, jsonOutput) + }, + } + cmd.Flags().BoolVar(&jsonOutput, "json", false, "output as JSON") + return cmd +} + +func skillsAccessCmd() *cobra.Command { + cmd := &cobra.Command{Use: "access", Short: "Manage skill access mode and effective access"} + cmd.AddCommand(skillsAccessGetCmd()) + cmd.AddCommand(skillsAccessSetCmd()) + cmd.AddCommand(skillsAccessEffectiveCmd()) + return cmd +} + +func skillsAccessGetCmd() *cobra.Command { + var jsonOutput bool + cmd := &cobra.Command{ + Use: "get [skill-id]", + Short: "Show skill access mode and grants", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + skillsRequireGateway() + path := "/v1/skills/" + url.PathEscape(args[0]) + "/access" + return runSkillsGateway(cmd.OutOrStdout(), http.MethodGet, path, nil, jsonOutput) + }, + } + cmd.Flags().BoolVar(&jsonOutput, "json", false, "output as JSON") + return cmd +} + +func skillsAccessSetCmd() *cobra.Command { + var mode string + var jsonOutput bool + cmd := &cobra.Command{ + Use: "set [skill-id]", + Short: "Set skill access mode", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if mode == "" { + return fmt.Errorf("--mode is required") + } + skillsRequireGateway() + path := "/v1/skills/" + url.PathEscape(args[0]) + "/access" + return runSkillsGateway(cmd.OutOrStdout(), http.MethodPatch, path, map[string]any{"mode": mode}, jsonOutput) + }, + } + cmd.Flags().StringVar(&mode, "mode", "", "access mode: private, internal, public") + cmd.Flags().BoolVar(&jsonOutput, "json", false, "output as JSON") + return cmd +} + +func skillsAccessEffectiveCmd() *cobra.Command { + var agentID, userID string + var jsonOutput bool + cmd := &cobra.Command{ + Use: "effective [skill-id]", + Short: "Inspect effective access for an agent and user", + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if agentID == "" || userID == "" { + return fmt.Errorf("--agent and --user are required") + } + skillsRequireGateway() + values := url.Values{"agent_id": {agentID}, "user_id": {userID}} + path := "/v1/skills/access/effective" + if len(args) == 1 { + path = "/v1/skills/" + url.PathEscape(args[0]) + "/access/effective" + } + return runSkillsGateway(cmd.OutOrStdout(), http.MethodGet, path+"?"+values.Encode(), nil, jsonOutput) + }, + } + cmd.Flags().StringVar(&agentID, "agent", "", "agent ID") + cmd.Flags().StringVar(&userID, "user", "", "user ID") + cmd.Flags().BoolVar(&jsonOutput, "json", false, "output as JSON") + return cmd +} + +func skillsGrantCmd() *cobra.Command { + cmd := &cobra.Command{Use: "grant", Short: "Grant skill access"} + cmd.AddCommand(skillsGrantAgentCmd()) + cmd.AddCommand(skillsGrantUserCmd()) + return cmd +} + +func skillsGrantAgentCmd() *cobra.Command { + var canManage, jsonOutput bool + var pinnedVersion int + cmd := &cobra.Command{ + Use: "agent [skill-id] [agent-id]", + Short: "Grant a skill to an agent", + Args: cobra.ExactArgs(2), + RunE: func(cmd *cobra.Command, args []string) error { + skillsRequireGateway() + body := map[string]any{"agent_id": args[1]} + if canManage { + body["can_manage"] = true + } + if pinnedVersion > 0 { + body["pinned_version"] = pinnedVersion + } + path := "/v1/skills/" + url.PathEscape(args[0]) + "/grants/agents" + return runSkillsGateway(cmd.OutOrStdout(), http.MethodPost, path, body, jsonOutput) + }, + } + cmd.Flags().BoolVar(&canManage, "can-manage", false, "grant manage permission") + cmd.Flags().IntVar(&pinnedVersion, "pinned-version", 0, "pin a specific skill version") + cmd.Flags().BoolVar(&jsonOutput, "json", false, "output as JSON") + return cmd +} diff --git a/cmd/skills_lifecycle_cmd_test.go b/cmd/skills_lifecycle_cmd_test.go new file mode 100644 index 00000000..b3a80f6a --- /dev/null +++ b/cmd/skills_lifecycle_cmd_test.go @@ -0,0 +1,159 @@ +package cmd + +import ( + "bytes" + "io" + "net/http" + "os" + "reflect" + "testing" +) + +func TestSkillsDepsInstallUsesPerSkillInstallEndpoint(t *testing.T) { + calls := captureSkillGatewayCalls(t) + + cmd := skillsDepsCmd() + cmd.SetArgs([]string{"install", "skill-123", "--json"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + if len(calls.requests) != 1 { + t.Fatalf("requests = %d, want 1", len(calls.requests)) + } + got := calls.requests[0] + if got.method != http.MethodPost || got.path != "/v1/skills/skill-123/dependencies/install" { + t.Fatalf("request = %+v", got) + } +} + +func TestSkillsAccessSetUsesPatchModeBody(t *testing.T) { + calls := captureSkillGatewayCalls(t) + + cmd := skillsAccessCmd() + cmd.SetArgs([]string{"set", "skill-123", "--mode", "public", "--json"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + if len(calls.requests) != 1 { + t.Fatalf("requests = %d, want 1", len(calls.requests)) + } + got := calls.requests[0] + if got.method != http.MethodPatch || got.path != "/v1/skills/skill-123/access" { + t.Fatalf("request = %+v", got) + } + if !reflect.DeepEqual(got.body, map[string]any{"mode": "public"}) { + t.Fatalf("body = %#v", got.body) + } +} + +func TestSkillsGrantAgentUsesPluralGrantEndpoint(t *testing.T) { + calls := captureSkillGatewayCalls(t) + + cmd := skillsGrantCmd() + cmd.SetArgs([]string{"agent", "skill-123", "agent-456", "--can-manage", "--pinned-version", "7", "--json"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + if len(calls.requests) != 1 { + t.Fatalf("requests = %d, want 1", len(calls.requests)) + } + got := calls.requests[0] + if got.method != http.MethodPost || got.path != "/v1/skills/skill-123/grants/agents" { + t.Fatalf("request = %+v", got) + } + want := map[string]any{"agent_id": "agent-456", "can_manage": true, "pinned_version": 7} + if !reflect.DeepEqual(got.body, want) { + t.Fatalf("body = %#v, want %#v", got.body, want) + } +} + +func TestSkillsAccessEffectiveBuildsOptionalSkillURL(t *testing.T) { + calls := captureSkillGatewayCalls(t) + + cmd := skillsAccessCmd() + cmd.SetArgs([]string{"effective", "skill-123", "--agent", "agent-456", "--user", "user-789", "--json"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + if len(calls.requests) != 1 { + t.Fatalf("requests = %d, want 1", len(calls.requests)) + } + got := calls.requests[0] + wantPath := "/v1/skills/skill-123/access/effective?agent_id=agent-456&user_id=user-789" + if got.method != http.MethodGet || got.path != wantPath { + t.Fatalf("request = %+v, want path %s", got, wantPath) + } +} + +type skillGatewayCall struct { + method string + path string + body any +} + +type capturedSkillGatewayCalls struct { + requests []skillGatewayCall +} + +func captureSkillGatewayCalls(t *testing.T) *capturedSkillGatewayCalls { + t.Helper() + calls := &capturedSkillGatewayCalls{} + prevDo := skillsGatewayDo + prevDelete := skillsGatewayDelete + prevRequire := skillsRequireGateway + prevStdout := os.Stdout + + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + os.Stdout = w + t.Cleanup(func() { + _ = w.Close() + _, _ = io.Copy(io.Discard, r) + _ = r.Close() + os.Stdout = prevStdout + skillsGatewayDo = prevDo + skillsGatewayDelete = prevDelete + skillsRequireGateway = prevRequire + }) + + skillsRequireGateway = func() {} + skillsGatewayDo = func(method, path string, body any) (map[string]any, error) { + calls.requests = append(calls.requests, skillGatewayCall{method: method, path: path, body: body}) + return map[string]any{"ok": true}, nil + } + skillsGatewayDelete = func(path string) error { + calls.requests = append(calls.requests, skillGatewayCall{method: http.MethodDelete, path: path}) + return nil + } + + return calls +} + +func TestRunLocalSkillDepsStatusPrintsJSON(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(dir+"/SKILL.md", []byte("---\nname: Demo\ndeps:\n - system:goclaw-missing-test-bin\n---\n"), 0o644); err != nil { + t.Fatalf("write SKILL.md: %v", err) + } + + var out bytes.Buffer + if err := runLocalSkillDepsStatus(&out, dir, true); err != nil { + t.Fatalf("runLocalSkillDepsStatus: %v", err) + } + if !bytes.Contains(out.Bytes(), []byte(`"missing_count": 1`)) { + t.Fatalf("output = %s", out.String()) + } +} diff --git a/cmd/skills_lifecycle_helpers.go b/cmd/skills_lifecycle_helpers.go new file mode 100644 index 00000000..2953c54b --- /dev/null +++ b/cmd/skills_lifecycle_helpers.go @@ -0,0 +1,134 @@ +package cmd + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "os" + "path/filepath" + + "github.com/spf13/cobra" + + "github.com/nextlevelbuilder/goclaw/internal/skills" +) + +func skillsGrantUserCmd() *cobra.Command { + var jsonOutput bool + cmd := &cobra.Command{ + Use: "user [skill-id] [user-id]", + Short: "Grant a skill to a user", + Args: cobra.ExactArgs(2), + RunE: func(cmd *cobra.Command, args []string) error { + skillsRequireGateway() + path := "/v1/skills/" + url.PathEscape(args[0]) + "/grants/users" + return runSkillsGateway(cmd.OutOrStdout(), http.MethodPost, path, map[string]any{"user_id": args[1]}, jsonOutput) + }, + } + cmd.Flags().BoolVar(&jsonOutput, "json", false, "output as JSON") + return cmd +} + +func skillsRevokeCmd() *cobra.Command { + cmd := &cobra.Command{Use: "revoke", Short: "Revoke skill access"} + cmd.AddCommand(skillsRevokeAgentCmd()) + cmd.AddCommand(skillsRevokeUserCmd()) + return cmd +} + +func skillsRevokeAgentCmd() *cobra.Command { + return &cobra.Command{ + Use: "agent [skill-id] [agent-id]", + Short: "Revoke a skill from an agent", + Args: cobra.ExactArgs(2), + RunE: func(cmd *cobra.Command, args []string) error { + skillsRequireGateway() + path := "/v1/skills/" + url.PathEscape(args[0]) + "/grants/agents/" + url.PathEscape(args[1]) + return runSkillsGatewayDelete(cmd.OutOrStdout(), path) + }, + } +} + +func skillsRevokeUserCmd() *cobra.Command { + return &cobra.Command{ + Use: "user [skill-id] [user-id]", + Short: "Revoke a skill from a user", + Args: cobra.ExactArgs(2), + RunE: func(cmd *cobra.Command, args []string) error { + skillsRequireGateway() + path := "/v1/skills/" + url.PathEscape(args[0]) + "/grants/users/" + url.PathEscape(args[1]) + return runSkillsGatewayDelete(cmd.OutOrStdout(), path) + }, + } +} + +func runSkillsGateway(w io.Writer, method, path string, body any, jsonOutput bool) error { + resp, err := skillsGatewayDo(method, path, body) + if err != nil { + return err + } + if jsonOutput { + return writePrettyJSON(w, resp) + } + if ok, _ := resp["ok"].(bool); ok { + _, _ = fmt.Fprintln(w, "ok") + return nil + } + return writePrettyJSON(w, resp) +} + +func runSkillsGatewayDelete(w io.Writer, path string) error { + if err := skillsGatewayDelete(path); err != nil { + return err + } + _, _ = fmt.Fprintln(w, "ok") + return nil +} + +func runLocalSkillDepsStatus(w io.Writer, target string, jsonOutput bool) error { + dir := target + if filepath.Base(target) == "SKILL.md" { + dir = filepath.Dir(target) + } + manifest := skills.ScanSkillDeps(dir) + ok, missing := skills.CheckSkillDeps(manifest) + resp := map[string]any{ + "skill": map[string]any{"path": dir}, + "ok": ok, + "status": localDepsStatus(ok), + "manifest": manifest, + "missing": missing, + "missing_count": len(missing), + } + if jsonOutput { + return writePrettyJSON(w, resp) + } + if ok { + _, _ = fmt.Fprintln(w, "all deps satisfied") + return nil + } + _, _ = fmt.Fprintf(w, "missing dependencies: %s\n", skills.FormatMissing(missing)) + return nil +} + +func writePrettyJSON(w io.Writer, v any) error { + data, err := json.MarshalIndent(v, "", " ") + if err != nil { + return err + } + _, err = fmt.Fprintln(w, string(data)) + return err +} + +func pathExists(path string) bool { + _, err := os.Stat(path) + return err == nil +} + +func localDepsStatus(ok bool) string { + if ok { + return "ok" + } + return "missing" +} diff --git a/docs/21-agent-evolution-and-skill-management.md b/docs/21-agent-evolution-and-skill-management.md index b535df5c..d1daf187 100644 --- a/docs/21-agent-evolution-and-skill-management.md +++ b/docs/21-agent-evolution-and-skill-management.md @@ -345,10 +345,18 @@ All endpoints require authentication (`authMiddleware`). Mutation endpoints requ | `PUT` | `/v1/skills/{id}` | Update metadata (owner/admin) | | `DELETE` | `/v1/skills/{id}` | Delete/archive skill (owner/admin) | | `POST` | `/v1/skills/{id}/toggle` | Enable/disable skill (owner/admin) | +| `GET` | `/v1/skills/{id}/dependencies` | Structured dependency status by source | +| `POST` | `/v1/skills/{id}/dependencies/scan` | Re-scan skill dependencies | +| `POST` | `/v1/skills/{id}/dependencies/check` | Check missing skill dependencies | +| `POST` | `/v1/skills/{id}/dependencies/install` | Install missing deps for one skill (master tenant) | +| `GET` | `/v1/skills/{id}/access` | Read visibility and grants | +| `PATCH` | `/v1/skills/{id}/access` | Set visibility/access mode | +| `GET` | `/v1/skills/{id}/access/effective` | Explain access for one skill/agent/user | +| `GET` | `/v1/skills/access/effective` | Explain effective access across skills | | `POST` | `/v1/skills/{id}/grants/agent` | Grant skill to agent (owner/admin) | -| `DELETE` | `/v1/skills/{id}/grants/agent` | Revoke agent grant (owner/admin) | +| `DELETE` | `/v1/skills/{id}/grants/agent/{agentID}` | Revoke agent grant (owner/admin) | | `POST` | `/v1/skills/{id}/grants/user` | Grant skill to user (owner/admin) | -| `DELETE` | `/v1/skills/{id}/grants/user` | Revoke user grant (owner/admin) | +| `DELETE` | `/v1/skills/{id}/grants/user/{userID}` | Revoke user grant (owner/admin) | | `POST` | `/v1/skills/upload` | Upload custom skill ZIP | | `POST` | `/v1/skills/rescan-deps` | Re-scan all enabled skills | | `POST` | `/v1/skills/install-deps` | Install all missing deps | diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 3b820c20..ffeba925 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -6,6 +6,24 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-06-12 +### Skill lifecycle API and CLI (issue #159) + +**Changes** + +- Added per-skill dependency scan/check/install endpoints and CLI commands. +- Added access read/update, agent/user grant aliases, and effective-access + inspection for operator workflows. +- Dependency status now reports system, pip, npm, and GitHub release deps in + structured output. +- Skill user grants are now tenant-scoped so shared system skills can be + granted to the same user ID in different tenants without conflicts. + +**Tests** + +- Added HTTP, CLI, PostgreSQL, and SQLite coverage for dependency lifecycle, + access mode changes, grants, effective access, grant-driven visibility, and + tenant-scoped user grants. + ### Mid-flight request preservation (issue #137) **Fixes** diff --git a/internal/http/skills.go b/internal/http/skills.go index 7d04a4e5..f9d93f3c 100644 --- a/internal/http/skills.go +++ b/internal/http/skills.go @@ -92,12 +92,26 @@ func (h *SkillsHandler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("POST /v1/skills/upload", h.adminMiddleware(h.handleUpload)) mux.HandleFunc("PUT /v1/skills/{id}", h.adminMiddleware(h.handleUpdate)) mux.HandleFunc("DELETE /v1/skills/{id}", h.adminMiddleware(h.handleDelete)) + mux.HandleFunc("GET /v1/skills/{id}/dependencies", h.adminMiddleware(h.handleSkillDependenciesStatus)) + mux.HandleFunc("POST /v1/skills/{id}/dependencies/scan", h.adminMiddleware(h.handleSkillDependenciesStatus)) + mux.HandleFunc("POST /v1/skills/{id}/dependencies/check", h.adminMiddleware(h.handleSkillDependenciesStatus)) + mux.HandleFunc("POST /v1/skills/{id}/dependencies/install", h.adminMiddleware(h.handleSkillDependenciesInstall)) + mux.HandleFunc("GET /v1/skills/{id}/access", h.adminMiddleware(h.handleGetSkillAccess)) + mux.HandleFunc("PATCH /v1/skills/{id}/access", h.adminMiddleware(h.handlePatchSkillAccess)) + mux.HandleFunc("GET /v1/skills/{id}/access/effective", h.adminMiddleware(h.handleGetSkillEffectiveAccess)) + mux.HandleFunc("GET /v1/skills/access/effective", h.adminMiddleware(h.handleListEffectiveAccess)) // Skill grants (admin+) mux.HandleFunc("GET /v1/skills/{id}/grants/agent", h.adminMiddleware(h.handleListAgentGrants)) mux.HandleFunc("POST /v1/skills/{id}/grants/agent", h.adminMiddleware(h.handleGrantAgent)) mux.HandleFunc("DELETE /v1/skills/{id}/grants/agent/{agentID}", h.adminMiddleware(h.handleRevokeAgent)) + mux.HandleFunc("GET /v1/skills/{id}/grants/agents", h.adminMiddleware(h.handleListAgentGrants)) + mux.HandleFunc("POST /v1/skills/{id}/grants/agents", h.adminMiddleware(h.handleGrantAgent)) + mux.HandleFunc("DELETE /v1/skills/{id}/grants/agents/{agentID}", h.adminMiddleware(h.handleRevokeAgent)) + mux.HandleFunc("GET /v1/skills/{id}/grants/users", h.adminMiddleware(h.handleListUserGrants)) mux.HandleFunc("POST /v1/skills/{id}/grants/user", h.adminMiddleware(h.handleGrantUser)) mux.HandleFunc("DELETE /v1/skills/{id}/grants/user/{userID}", h.adminMiddleware(h.handleRevokeUser)) + mux.HandleFunc("POST /v1/skills/{id}/grants/users", h.adminMiddleware(h.handleGrantUser)) + mux.HandleFunc("DELETE /v1/skills/{id}/grants/users/{userID}", h.adminMiddleware(h.handleRevokeUser)) // System-level operations: admin + master tenant only. // These execute shell commands (pip/npm install) and affect the entire server. mux.HandleFunc("POST /v1/skills/rescan-deps", h.adminMiddleware(h.handleRescanDeps)) @@ -128,19 +142,7 @@ func (h *SkillsHandler) adminMiddleware(next http.HandlerFunc) http.HandlerFunc // System skill management (install packages, rescan deps) is a server-wide operation // that should only be accessible to the master tenant or cross-tenant admins. func (h *SkillsHandler) requireMasterTenant(w http.ResponseWriter, r *http.Request) bool { - ctx := r.Context() - if store.IsOwnerRole(ctx) { - return true - } - tid := store.TenantIDFromContext(ctx) - if tid == store.MasterTenantID { - return true - } - locale := store.LocaleFromContext(ctx) - writeJSON(w, http.StatusForbidden, map[string]string{ - "error": i18n.T(locale, i18n.MsgPermissionDenied, "system skill management"), - }) - return false + return requireMasterScope(w, r) } func (h *SkillsHandler) handleList(w http.ResponseWriter, r *http.Request) { @@ -503,7 +505,7 @@ func stringSlicesEqual(a, b []string) bool { // bundled dir if the managed copy's scripts/ directory is missing or empty. // If a fallback scan succeeds, re-copies the bundled scripts to the managed dir. func (h *SkillsHandler) scanWithFallback(sk store.SkillInfo) *skills.SkillManifest { - manifest := skills.ScanSkillDeps(sk.BaseDir) + manifest := scanSkillDeps(sk.BaseDir) if manifest != nil && !manifest.IsEmpty() { return manifest } @@ -520,7 +522,7 @@ func (h *SkillsHandler) scanWithFallback(sk store.SkillInfo) *skills.SkillManife } bundledSkillDir := filepath.Join(h.bundledDir, sk.Slug) - bundledManifest := skills.ScanSkillDeps(bundledSkillDir) + bundledManifest := scanSkillDeps(bundledSkillDir) if bundledManifest == nil || bundledManifest.IsEmpty() { return manifest } diff --git a/internal/http/skills_access.go b/internal/http/skills_access.go new file mode 100644 index 00000000..b7ff91c0 --- /dev/null +++ b/internal/http/skills_access.go @@ -0,0 +1,152 @@ +package http + +import ( + "net/http" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/i18n" + "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +type skillAccessResponse struct { + Skill skillRef `json:"skill"` + Visibility string `json:"visibility"` + AgentGrants []store.SkillAgentGrantInfo `json:"agent_grants"` + UserGrants []store.SkillUserGrantInfo `json:"user_grants"` +} + +type skillEffectiveAccessResponse struct { + Skill skillRef `json:"skill"` + Accessible bool `json:"accessible"` + Reason string `json:"reason"` + CanManage bool `json:"can_manage"` + PinnedVersion *int `json:"pinned_version,omitempty"` +} + +func (h *SkillsHandler) handleGetSkillAccess(w http.ResponseWriter, r *http.Request) { + sk, ok := h.lifecycleSkill(w, r, false) + if !ok { + return + } + agentGrants, err := h.skills.ListAgentGrantsForSkill(r.Context(), mustParseUUID(sk.ID)) + if err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + return + } + userGrants, err := h.skills.ListUserGrantsForSkill(r.Context(), mustParseUUID(sk.ID)) + if err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + return + } + writeJSON(w, http.StatusOK, skillAccessResponse{ + Skill: refForSkill(sk), + Visibility: sk.Visibility, + AgentGrants: agentGrants, + UserGrants: userGrants, + }) +} + +func (h *SkillsHandler) handlePatchSkillAccess(w http.ResponseWriter, r *http.Request) { + locale := store.LocaleFromContext(r.Context()) + sk, ok := h.lifecycleSkill(w, r, false) + if !ok { + return + } + if sk.IsSystem && !h.requireMasterTenant(w, r) { + return + } + var req struct { + Mode string `json:"mode"` + Visibility string `json:"visibility"` + } + if !bindJSON(w, r, locale, &req) { + return + } + visibility := req.Visibility + if visibility == "" { + visibility = req.Mode + } + if err := skills.ValidateVisibility(visibility); err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidVisibility, visibility)}) + return + } + visibility = skills.NormalizeVisibility(visibility) + id := mustParseUUID(sk.ID) + updateCtx := r.Context() + if sk.IsSystem { + updateCtx = store.WithCrossTenant(store.WithTenantID(updateCtx, store.MasterTenantID)) + } + if err := h.skills.UpdateSkill(updateCtx, id, map[string]any{"visibility": visibility}); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + return + } + h.skills.BumpVersion() + h.emitCacheInvalidate(bus.CacheKindSkills, sk.ID, uuid.Nil) + emitAudit(h.msgBus, r, "skill.access_updated", "skill", sk.ID) + writeJSON(w, http.StatusOK, map[string]any{"ok": true, "visibility": visibility}) +} + +func (h *SkillsHandler) handleGetSkillEffectiveAccess(w http.ResponseWriter, r *http.Request) { + sk, ok := h.lifecycleSkill(w, r, false) + if !ok { + return + } + agentID, userID, ok := parseEffectiveAccessParams(w, r) + if !ok { + return + } + if !h.requireTenantUser(w, r, userID) { + return + } + idx, err := h.buildEffectiveAccessIndex(r, agentID, userID) + if err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + return + } + resp := effectiveAccessForSkill(r.Context(), sk, idx, userID) + writeJSON(w, http.StatusOK, resp) +} + +func (h *SkillsHandler) handleListEffectiveAccess(w http.ResponseWriter, r *http.Request) { + agentID, userID, ok := parseEffectiveAccessParams(w, r) + if !ok { + return + } + if !h.requireTenantUser(w, r, userID) { + return + } + idx, err := h.buildEffectiveAccessIndex(r, agentID, userID) + if err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + return + } + skills := h.skills.ListAllSkills(r.Context()) + out := make([]skillEffectiveAccessResponse, 0, len(skills)) + for _, sk := range skills { + out = append(out, effectiveAccessForSkill(r.Context(), sk, idx, userID)) + } + writeJSON(w, http.StatusOK, map[string]any{"skills": out}) +} + +func parseEffectiveAccessParams(w http.ResponseWriter, r *http.Request) (uuid.UUID, string, bool) { + locale := store.LocaleFromContext(r.Context()) + agentID, err := uuid.Parse(r.URL.Query().Get("agent_id")) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "agent")}) + return uuid.Nil, "", false + } + userID := r.URL.Query().Get("user_id") + if err := store.ValidateUserID(userID); err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()}) + return uuid.Nil, "", false + } + return agentID, userID, true +} + +func mustParseUUID(raw string) uuid.UUID { + id, _ := uuid.Parse(raw) + return id +} diff --git a/internal/http/skills_access_effective.go b/internal/http/skills_access_effective.go new file mode 100644 index 00000000..20a7f2b5 --- /dev/null +++ b/internal/http/skills_access_effective.go @@ -0,0 +1,111 @@ +package http + +import ( + "context" + "fmt" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +type effectiveAccessIndex struct { + actorID string + accessibleBySlug map[string]bool + agentGrantByID map[string]store.SkillWithGrantStatus + agentGrantBySlug map[string]store.SkillWithGrantStatus +} + +func (h *SkillsHandler) buildEffectiveAccessIndex(r interface{ Context() context.Context }, agentID uuid.UUID, userID string) (effectiveAccessIndex, error) { + ctx := r.Context() + accessStore, ok := h.skills.(store.SkillAccessStore) + if !ok { + return effectiveAccessIndex{}, fmt.Errorf("skill access store not available") + } + accessible, err := accessStore.ListAccessible(ctx, agentID, userID) + if err != nil { + return effectiveAccessIndex{}, err + } + grantStatus, err := h.skills.ListWithGrantStatus(ctx, agentID) + if err != nil { + return effectiveAccessIndex{}, err + } + idx := effectiveAccessIndex{ + actorID: store.ActorIDFromContext(ctx), + accessibleBySlug: make(map[string]bool, len(accessible)), + agentGrantByID: make(map[string]store.SkillWithGrantStatus, len(grantStatus)), + agentGrantBySlug: make(map[string]store.SkillWithGrantStatus, len(grantStatus)), + } + if idx.actorID == "" { + idx.actorID = userID + } + for _, sk := range accessible { + idx.accessibleBySlug[sk.Slug] = true + } + for _, grant := range grantStatus { + idx.agentGrantByID[grant.ID.String()] = grant + idx.agentGrantBySlug[grant.Slug] = grant + } + return idx, nil +} + +func effectiveAccessForSkill(ctx context.Context, sk store.SkillInfo, idx effectiveAccessIndex, userID string) skillEffectiveAccessResponse { + resp := skillEffectiveAccessResponse{Skill: refForSkill(sk), Reason: "none"} + if sk.Status != "active" || !sk.Enabled { + resp.Reason = "inactive" + return resp + } + if !idx.accessible(sk) { + return resp + } + if sk.IsSystem { + resp.Accessible = true + resp.Reason = "system" + return resp + } + if sk.Visibility == skills.VisibilityPublic { + resp.Accessible = true + resp.Reason = "public" + return resp + } + actorID := idx.actorID + if actorID == "" { + actorID = store.ActorIDFromContext(ctx) + } + if actorID == "" { + actorID = userID + } + if sk.Visibility == skills.VisibilityPrivate && (sk.OwnerID == userID || sk.OwnerID == actorID) { + resp.Accessible = true + resp.Reason = "owner" + return resp + } + if sk.Visibility != skills.VisibilityInternal { + return resp + } + if grant, ok := idx.agentGrant(sk); ok && grant.Granted { + resp.Accessible = true + resp.Reason = "agent_grant" + resp.CanManage = grant.CanManage + resp.PinnedVersion = grant.PinnedVer + return resp + } + resp.Accessible = true + resp.Reason = "user_grant" + return resp +} + +func (idx effectiveAccessIndex) accessible(sk store.SkillInfo) bool { + return idx.accessibleBySlug[sk.Slug] +} + +func (idx effectiveAccessIndex) agentGrant(sk store.SkillInfo) (store.SkillWithGrantStatus, bool) { + if sk.ID != "" { + if grant, ok := idx.agentGrantByID[sk.ID]; ok { + return grant, true + } + } + grant, ok := idx.agentGrantBySlug[sk.Slug] + return grant, ok +} diff --git a/internal/http/skills_grants.go b/internal/http/skills_grants.go index 60ce877b..3b307325 100644 --- a/internal/http/skills_grants.go +++ b/internal/http/skills_grants.go @@ -50,6 +50,25 @@ func (h *SkillsHandler) handleListAgentGrants(w http.ResponseWriter, r *http.Req writeJSON(w, http.StatusOK, map[string]any{"grants": grants}) } +func (h *SkillsHandler) handleListUserGrants(w http.ResponseWriter, r *http.Request) { + locale := store.LocaleFromContext(r.Context()) + idStr := r.PathValue("id") + skillID, err := uuid.Parse(idStr) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "skill")}) + return + } + + grants, err := h.skills.ListUserGrantsForSkill(r.Context(), skillID) + if err != nil { + slog.Error("failed to list skill user grants", "skill_id", skillID, "error", err) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgFailedToList, "skill grants")}) + return + } + + writeJSON(w, http.StatusOK, map[string]any{"grants": grants}) +} + func (h *SkillsHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) { locale := store.LocaleFromContext(r.Context()) userID := store.UserIDFromContext(r.Context()) @@ -70,9 +89,10 @@ func (h *SkillsHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) } var req struct { - AgentID string `json:"agent_id"` - Version int `json:"version"` - CanManage *bool `json:"can_manage"` + AgentID string `json:"agent_id"` + Version int `json:"version"` + PinnedVersion int `json:"pinned_version"` + CanManage *bool `json:"can_manage"` } if !bindJSON(w, r, locale, &req) { return @@ -84,6 +104,9 @@ func (h *SkillsHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) return } + if req.PinnedVersion > 0 { + req.Version = req.PinnedVersion + } if req.Version <= 0 { req.Version = 1 } @@ -175,6 +198,9 @@ func (h *SkillsHandler) handleGrantUser(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()}) return } + if !h.requireTenantUser(w, r, req.UserID) { + return + } if err := h.skills.GrantToUser(r.Context(), skillID, req.UserID, userID); err != nil { writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) @@ -222,4 +248,31 @@ func (h *SkillsHandler) handleRevokeUser(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, map[string]string{"ok": "true"}) } +func (h *SkillsHandler) requireTenantUser(w http.ResponseWriter, r *http.Request, userID string) bool { + locale := store.LocaleFromContext(r.Context()) + if h.tenantStore == nil { + writeJSON(w, http.StatusNotImplemented, map[string]string{"error": i18n.T(locale, i18n.MsgNotImplemented, "tenant store")}) + return false + } + tenantID := store.TenantIDFromContext(r.Context()) + if tenantID == uuid.Nil { + tenantID = store.MasterTenantID + } + role, err := h.tenantStore.GetUserRole(r.Context(), tenantID, userID) + if err != nil { + slog.Error("skill_grants: tenant user membership check failed", "tenant_id", tenantID, "target_user_id", userID, "error", err) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgFailedToList, "tenant users")}) + return false + } + if role == "" { + slog.Warn("security.skill_grant_user_not_tenant_member", + "tenant_id", tenantID, + "target_user_id", userID, + "user_id", store.UserIDFromContext(r.Context())) + writeJSON(w, http.StatusForbidden, map[string]string{"error": i18n.T(locale, i18n.MsgPermissionDenied, "tenant user")}) + return false + } + return true +} + // --- Helpers --- diff --git a/internal/http/skills_lifecycle.go b/internal/http/skills_lifecycle.go new file mode 100644 index 00000000..34dd63b4 --- /dev/null +++ b/internal/http/skills_lifecycle.go @@ -0,0 +1,144 @@ +package http + +import ( + "context" + "net/http" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/i18n" + "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +var ( + scanSkillDeps = skills.ScanSkillDeps + checkSkillDeps = skills.CheckSkillDeps + githubSkillDependencyInstalled = skillGitHubDependencyInstalled +) + +type skillRef struct { + ID string `json:"id"` + Slug string `json:"slug"` + Name string `json:"name"` + Status string `json:"status"` + Version int `json:"version"` +} + +type skillDependencyItem struct { + Source string `json:"source"` + Name string `json:"name"` + Status string `json:"status"` +} + +type skillDependencyStatusResponse struct { + Skill skillRef `json:"skill"` + Dependencies []skillDependencyItem `json:"dependencies"` + OK bool `json:"ok"` + Status string `json:"status"` + Missing []string `json:"missing,omitempty"` + MissingCount int `json:"missing_count"` +} + +func (h *SkillsHandler) handleSkillDependenciesStatus(w http.ResponseWriter, r *http.Request) { + if !h.requireMasterTenant(w, r) { + return + } + sk, ok := h.lifecycleSkill(w, r, false) + if !ok { + return + } + writeJSON(w, http.StatusOK, h.buildSkillDependencyStatus(sk)) +} + +func (h *SkillsHandler) handleSkillDependenciesInstall(w http.ResponseWriter, r *http.Request) { + if !h.requireMasterTenant(w, r) { + return + } + sk, ok := h.lifecycleSkill(w, r, true) + if !ok { + return + } + manifest := h.scanWithFallback(sk) + if manifest == nil || manifest.IsEmpty() { + h.persistSkillDependencyState(r.Context(), sk, true, nil) + writeJSON(w, http.StatusOK, h.buildSkillDependencyStatus(sk)) + return + } + _, missing := lifecycleCheckSkillDeps(manifest) + if len(missing) == 0 { + h.persistSkillDependencyState(r.Context(), sk, true, nil) + writeJSON(w, http.StatusOK, h.buildSkillDependencyStatus(sk)) + return + } + result, err := installLifecycleDeps(r.Context(), manifest, missing) + if err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + return + } + okAfter, missingAfter := lifecycleCheckSkillDeps(manifest) + h.persistSkillDependencyState(r.Context(), sk, okAfter, missingAfter) + resp := h.buildSkillDependencyStatus(sk) + resp.OK = okAfter + resp.Missing = missingAfter + resp.MissingCount = len(missingAfter) + resp.Status = dependencyStatus(okAfter) + writeJSON(w, http.StatusOK, map[string]any{"result": result, "dependencies": resp}) +} + +func (h *SkillsHandler) lifecycleSkill(w http.ResponseWriter, r *http.Request, crossTenant bool) (store.SkillInfo, bool) { + locale := store.LocaleFromContext(r.Context()) + id, err := uuid.Parse(r.PathValue("id")) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "skill")}) + return store.SkillInfo{}, false + } + ctx := r.Context() + if crossTenant { + ctx = store.WithCrossTenant(store.WithTenantID(ctx, store.MasterTenantID)) + } + sk, ok := h.skills.GetSkillByID(ctx, id) + if !ok { + writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "skill", id.String())}) + return store.SkillInfo{}, false + } + return sk, true +} + +func (h *SkillsHandler) buildSkillDependencyStatus(sk store.SkillInfo) skillDependencyStatusResponse { + manifest := h.scanWithFallback(sk) + if manifest == nil || manifest.IsEmpty() { + return skillDependencyStatusResponse{Skill: refForSkill(sk), OK: true, Status: "ok"} + } + ok, missing := lifecycleCheckSkillDeps(manifest) + return skillDependencyStatusResponse{ + Skill: refForSkill(sk), + Dependencies: dependencyItems(manifest, missing), + OK: ok, + Status: dependencyStatus(ok), + Missing: missing, + MissingCount: len(missing), + } +} + +func (h *SkillsHandler) persistSkillDependencyState(ctx context.Context, sk store.SkillInfo, ok bool, missing []string) { + id, err := uuid.Parse(sk.ID) + if err != nil { + return + } + updateCtx := skillTenantContext(ctx, sk) + _ = h.skills.StoreMissingDeps(updateCtx, id, missing) + wantStatus := "archived" + if ok { + wantStatus = "active" + } + if sk.Status != wantStatus { + _ = h.skills.UpdateSkill(updateCtx, id, map[string]any{"status": wantStatus}) + h.emitCacheInvalidate(bus.CacheKindSkills, sk.ID, uuid.Nil) + } +} + +func refForSkill(sk store.SkillInfo) skillRef { + return skillRef{ID: sk.ID, Slug: sk.Slug, Name: sk.Name, Status: sk.Status, Version: sk.Version} +} diff --git a/internal/http/skills_lifecycle_deps.go b/internal/http/skills_lifecycle_deps.go new file mode 100644 index 00000000..f32a447e --- /dev/null +++ b/internal/http/skills_lifecycle_deps.go @@ -0,0 +1,164 @@ +package http + +import ( + "context" + "strings" + + "github.com/nextlevelbuilder/goclaw/internal/skills" +) + +func lifecycleCheckSkillDeps(m *skills.SkillManifest) (bool, []string) { + ok, missing := checkSkillDeps(m) + missing = appendUniqueDeps(missing, missingGitHubSkillDeps(m)...) + return ok && len(missing) == 0, missing +} + +func missingGitHubSkillDeps(m *skills.SkillManifest) []string { + if m == nil { + return nil + } + var missing []string + for _, raw := range m.Explicit { + if strings.HasPrefix(raw, "github:") && !githubSkillDependencyInstalled(raw) { + missing = append(missing, raw) + } + } + return missing +} + +func skillGitHubDependencyInstalled(raw string) bool { + spec, err := skills.ParseGitHubSpec(raw) + if err != nil { + return false + } + installer := skills.DefaultGitHubInstaller() + if installer == nil { + return false + } + entries, err := installer.List() + if err != nil { + return false + } + wantRepo := spec.Owner + "/" + spec.Repo + for _, entry := range entries { + if strings.EqualFold(entry.Repo, wantRepo) && (spec.Tag == "" || entry.Tag == spec.Tag) { + return true + } + } + return false +} + +func installLifecycleDeps(ctx context.Context, manifest *skills.SkillManifest, missing []string) (*skills.InstallResult, error) { + bulkMissing, githubMissing := splitGitHubMissingDeps(missing) + result := &skills.InstallResult{} + if len(bulkMissing) > 0 { + bulkResult, err := installManagedDeps(ctx, manifest, bulkMissing) + if err != nil { + return nil, err + } + mergeInstallResult(result, bulkResult) + } + for _, dep := range githubMissing { + ok, errMsg := installSingleDep(ctx, dep) + name := strings.TrimPrefix(dep, "github:") + if !ok { + result.Errors = append(result.Errors, "github "+name+": "+errMsg) + continue + } + result.GitHub = append(result.GitHub, name) + } + return result, nil +} + +func splitGitHubMissingDeps(missing []string) ([]string, []string) { + var bulkMissing []string + var githubMissing []string + for _, dep := range missing { + if strings.HasPrefix(dep, "github:") { + githubMissing = append(githubMissing, dep) + continue + } + bulkMissing = append(bulkMissing, dep) + } + return bulkMissing, githubMissing +} + +func mergeInstallResult(dst, src *skills.InstallResult) { + if dst == nil || src == nil { + return + } + dst.System = append(dst.System, src.System...) + dst.Pip = append(dst.Pip, src.Pip...) + dst.Npm = append(dst.Npm, src.Npm...) + dst.GitHub = append(dst.GitHub, src.GitHub...) + dst.Errors = append(dst.Errors, src.Errors...) +} + +func dependencyItems(m *skills.SkillManifest, missing []string) []skillDependencyItem { + missingSet := dependencyMissingSet(missing) + var out []skillDependencyItem + add := func(source, name string) { + status := "installed" + if missingSet[dependencyKey(source, name)] { + status = "missing" + } + out = append(out, skillDependencyItem{Source: source, Name: name, Status: status}) + } + for _, name := range m.Requires { + add("system", name) + } + for _, name := range m.RequiresPython { + add("pip", name) + } + for _, name := range m.RequiresNode { + add("npm", name) + } + for _, raw := range m.Explicit { + if after, ok := strings.CutPrefix(raw, "github:"); ok { + add("github", after) + } + } + return out +} + +func dependencyMissingSet(missing []string) map[string]bool { + out := make(map[string]bool, len(missing)) + for _, dep := range missing { + source, name := splitDependency(dep) + out[dependencyKey(source, name)] = true + } + return out +} + +func splitDependency(dep string) (string, string) { + for _, prefix := range []string{"pip:", "npm:", "github:", "system:"} { + if after, ok := strings.CutPrefix(dep, prefix); ok { + return strings.TrimSuffix(prefix, ":"), after + } + } + return "system", dep +} + +func dependencyKey(source, name string) string { return source + ":" + name } + +func appendUniqueDeps(dst []string, src ...string) []string { + seen := make(map[string]bool, len(dst)+len(src)) + for _, dep := range dst { + seen[dep] = true + } + for _, dep := range src { + if seen[dep] { + continue + } + dst = append(dst, dep) + seen[dep] = true + } + return dst +} + +func dependencyStatus(ok bool) string { + if ok { + return "ok" + } + return "missing" +} diff --git a/internal/http/skills_lifecycle_test.go b/internal/http/skills_lifecycle_test.go new file mode 100644 index 00000000..de260a33 --- /dev/null +++ b/internal/http/skills_lifecycle_test.go @@ -0,0 +1,437 @@ +package http + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "path/filepath" + "reflect" + "strings" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/crypto" + "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +func TestHandleSkillDependenciesStatus_ReturnsStructuredMissingBySource(t *testing.T) { + handler, skillStore, ctx, root := newTestUploadHandler(t) + skillDir := filepath.Join(root, "skills-store", "dep-skill", "1") + skillID := skillStore.seedCustomSkill("dep-skill", skillDir, "archived", []string{"ffmpeg", "pip:requests"}) + + prevScan := scanSkillDeps + prevCheck := checkSkillDeps + prevGitHubInstalled := githubSkillDependencyInstalled + scanSkillDeps = func(string) *skills.SkillManifest { + return &skills.SkillManifest{ + Requires: []string{"ffmpeg"}, + RequiresPython: []string{"requests"}, + RequiresNode: []string{"tsx"}, + Explicit: []string{"github:cli/cli@v2.40.0"}, + } + } + checkSkillDeps = func(*skills.SkillManifest) (bool, []string) { + return false, []string{"ffmpeg", "pip:requests"} + } + githubSkillDependencyInstalled = func(string) bool { return false } + t.Cleanup(func() { + scanSkillDeps = prevScan + checkSkillDeps = prevCheck + githubSkillDependencyInstalled = prevGitHubInstalled + }) + + req := httptest.NewRequest(http.MethodGet, "/v1/skills/"+skillID.String()+"/dependencies", http.NoBody).WithContext(ctx) + req.SetPathValue("id", skillID.String()) + w := httptest.NewRecorder() + + handler.handleSkillDependenciesStatus(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + var resp struct { + OK bool `json:"ok"` + MissingCount int `json:"missing_count"` + Dependencies []struct { + Source string `json:"source"` + Name string `json:"name"` + Status string `json:"status"` + } `json:"dependencies"` + } + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if resp.OK { + t.Fatal("ok = true, want false") + } + if resp.MissingCount != 3 { + t.Fatalf("missing_count = %d, want 3", resp.MissingCount) + } + got := map[string]string{} + for _, dep := range resp.Dependencies { + got[dep.Source+":"+dep.Name] = dep.Status + } + want := map[string]string{ + "system:ffmpeg": "missing", + "pip:requests": "missing", + "npm:tsx": "installed", + "github:cli/cli@v2.40.0": "missing", + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("dependencies = %#v, want %#v", got, want) + } +} + +func TestSkillDependencyRoutesRequireAdmin(t *testing.T) { + handler, skillStore, _, root := newTestUploadHandler(t) + skillID := skillStore.seedCustomSkill("dep-skill", filepath.Join(root, "skills-store", "dep-skill", "1"), "active", nil) + tenantID := uuid.New() + setupTestCache(t, map[string]*store.APIKeyData{ + crypto.HashAPIKey("read-token"): {ID: uuid.New(), Scopes: []string{"operator.read"}}, + crypto.HashAPIKey("tenant-admin-token"): { + ID: uuid.New(), + Scopes: []string{"operator.admin"}, + TenantID: tenantID, + OwnerID: "tenant-admin", + }, + }) + + prevScan := scanSkillDeps + scanCalled := false + scanSkillDeps = func(string) *skills.SkillManifest { + scanCalled = true + return &skills.SkillManifest{} + } + t.Cleanup(func() { scanSkillDeps = prevScan }) + + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + req := httptest.NewRequest(http.MethodGet, "/v1/skills/"+skillID.String()+"/dependencies", http.NoBody) + req.Header.Set("Authorization", "Bearer read-token") + w := httptest.NewRecorder() + + mux.ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body = %s", w.Code, w.Body.String()) + } + if scanCalled { + t.Fatal("dependency scan ran for non-admin caller") + } + + scanCalled = false + req = httptest.NewRequest(http.MethodGet, "/v1/skills/"+skillID.String()+"/dependencies", http.NoBody) + req.Header.Set("Authorization", "Bearer tenant-admin-token") + w = httptest.NewRecorder() + + mux.ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Fatalf("tenant admin status = %d, want 403; body = %s", w.Code, w.Body.String()) + } + if scanCalled { + t.Fatal("dependency scan ran for non-master tenant admin") + } +} + +func TestHandleSkillDependenciesInstall_SplitsGitHubDepsToSingleInstaller(t *testing.T) { + handler, skillStore, ctx, root := newTestUploadHandler(t) + skillDir := filepath.Join(root, "skills-store", "dep-skill", "1") + skillID := skillStore.seedCustomSkill("dep-skill", skillDir, "archived", []string{"pip:requests", "github:cli/cli@v2.40.0"}) + + prevScan := scanSkillDeps + prevCheck := checkSkillDeps + prevInstallManaged := installManagedDeps + prevInstallSingle := installSingleDep + prevGitHubInstalled := githubSkillDependencyInstalled + githubInstalled := false + scanSkillDeps = func(string) *skills.SkillManifest { + return &skills.SkillManifest{ + RequiresPython: []string{"requests"}, + Explicit: []string{"github:cli/cli@v2.40.0"}, + } + } + checkSkillDeps = func(*skills.SkillManifest) (bool, []string) { + if githubInstalled { + return true, nil + } + return false, []string{"pip:requests"} + } + githubSkillDependencyInstalled = func(raw string) bool { + if raw != "github:cli/cli@v2.40.0" { + t.Fatalf("github status checked for %q", raw) + } + return githubInstalled + } + installManagedDeps = func(_ context.Context, _ *skills.SkillManifest, missing []string) (*skills.InstallResult, error) { + if !reflect.DeepEqual(missing, []string{"pip:requests"}) { + t.Fatalf("managed missing = %#v, want pip only", missing) + } + return &skills.InstallResult{Pip: []string{"requests"}}, nil + } + installSingleDep = func(_ context.Context, dep string) (bool, string) { + if dep != "github:cli/cli@v2.40.0" { + t.Fatalf("single dep = %q, want github spec", dep) + } + githubInstalled = true + return true, "" + } + t.Cleanup(func() { + scanSkillDeps = prevScan + checkSkillDeps = prevCheck + installManagedDeps = prevInstallManaged + installSingleDep = prevInstallSingle + githubSkillDependencyInstalled = prevGitHubInstalled + }) + + req := httptest.NewRequest(http.MethodPost, "/v1/skills/"+skillID.String()+"/dependencies/install", http.NoBody).WithContext(ctx) + req.SetPathValue("id", skillID.String()) + w := httptest.NewRecorder() + + handler.handleSkillDependenciesInstall(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + var resp struct { + Result skills.InstallResult `json:"result"` + Deps struct { + OK bool `json:"ok"` + MissingCount int `json:"missing_count"` + } `json:"dependencies"` + } + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if !reflect.DeepEqual(resp.Result.Pip, []string{"requests"}) || !reflect.DeepEqual(resp.Result.GitHub, []string{"cli/cli@v2.40.0"}) { + t.Fatalf("install result = %+v", resp.Result) + } + if !resp.Deps.OK || resp.Deps.MissingCount != 0 { + t.Fatalf("dependencies = %+v", resp.Deps) + } +} + +func TestHandleSkillDependenciesInstall_RejectsNonMasterTenant(t *testing.T) { + handler, skillStore, _, root := newTestUploadHandler(t) + tenantID := uuid.New() + ctx := store.WithTenantID(context.Background(), tenantID) + skillDir := filepath.Join(root, "tenants", tenantID.String(), "skills-store", "dep-skill", "1") + skillID := skillStore.seedCustomSkillForTenant(tenantID, "dep-skill", skillDir, "archived", []string{"pip:requests"}) + + installCalled := false + prevInstall := installManagedDeps + installManagedDeps = func(context.Context, *skills.SkillManifest, []string) (*skills.InstallResult, error) { + installCalled = true + return &skills.InstallResult{}, nil + } + t.Cleanup(func() { installManagedDeps = prevInstall }) + + req := httptest.NewRequest(http.MethodPost, "/v1/skills/"+skillID.String()+"/dependencies/install", http.NoBody).WithContext(ctx) + req.SetPathValue("id", skillID.String()) + w := httptest.NewRecorder() + + handler.handleSkillDependenciesInstall(w, req) + + if w.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body = %s", w.Code, w.Body.String()) + } + if installCalled { + t.Fatal("installManagedDeps was called for non-master tenant") + } +} + +func TestSkillGrantUserRouteRejectsNonTenantMember(t *testing.T) { + handler, skillStore, _, root := newTestUploadHandler(t) + tenantID := uuid.New() + skillID := skillStore.seedCustomSkillForTenant(tenantID, "access-skill", filepath.Join(root, "tenants", tenantID.String(), "skills-store", "access-skill", "1"), "active", nil) + tenants := newMockTenantStore() + tenants.addTenant(tenantID, "tenant-a") + tenants.setUserRole(tenantID, "admin-user", store.TenantRoleAdmin) + handler.tenantStore = tenants + setupTestCache(t, map[string]*store.APIKeyData{ + crypto.HashAPIKey("admin-token"): { + ID: uuid.New(), + Scopes: []string{"operator.admin"}, + TenantID: tenantID, + OwnerID: "admin-user", + }, + }) + + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + req := httptest.NewRequest(http.MethodPost, "/v1/skills/"+skillID.String()+"/grants/users", strings.NewReader(`{"user_id":"outside-user"}`)) + req.Header.Set("Authorization", "Bearer admin-token") + w := httptest.NewRecorder() + + mux.ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body = %s", w.Code, w.Body.String()) + } + if len(skillStore.userGrantCalls) != 0 { + t.Fatalf("user grant calls = %+v, want none", skillStore.userGrantCalls) + } +} + +func TestHandleSkillAccessGetListsAgentAndUserGrants(t *testing.T) { + handler, skillStore, ctx, root := newTestUploadHandler(t) + skillID := skillStore.seedCustomSkill("access-skill", filepath.Join(root, "skills-store", "access-skill", "1"), "active", nil) + agentID := uuid.New() + skillStore.agentGrants[skillID] = []store.SkillAgentGrantInfo{{ + AgentID: agentID, + PinnedVersion: 3, + GrantedBy: "owner-user", + CanManage: true, + }} + skillStore.userGrants[skillID] = []store.SkillUserGrantInfo{{ + UserID: "target-user", + GrantedBy: "owner-user", + }} + + req := httptest.NewRequest(http.MethodGet, "/v1/skills/"+skillID.String()+"/access", http.NoBody).WithContext(ctx) + req.SetPathValue("id", skillID.String()) + w := httptest.NewRecorder() + + handler.handleGetSkillAccess(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + var resp struct { + Visibility string `json:"visibility"` + AgentGrants []store.SkillAgentGrantInfo `json:"agent_grants"` + UserGrants []store.SkillUserGrantInfo `json:"user_grants"` + } + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if len(resp.AgentGrants) != 1 || !resp.AgentGrants[0].CanManage || resp.AgentGrants[0].PinnedVersion != 3 { + t.Fatalf("agent grants = %+v", resp.AgentGrants) + } + if len(resp.UserGrants) != 1 || resp.UserGrants[0].UserID != "target-user" { + t.Fatalf("user grants = %+v", resp.UserGrants) + } +} + +func TestHandleSkillEffectiveAccessReportsAgentGrant(t *testing.T) { + handler, skillStore, ctx, root := newTestUploadHandler(t) + skillID := skillStore.seedCustomSkill("effective-skill", filepath.Join(root, "skills-store", "effective-skill", "1"), "active", nil) + agentID := uuid.New() + tenants := newMockTenantStore() + tenants.addTenant(store.MasterTenantID, "master") + tenants.setUserRole(store.MasterTenantID, "target-user", store.TenantRoleMember) + handler.tenantStore = tenants + skillStore.skills[skillID] = store.SkillInfo{ + ID: skillID.String(), + TenantID: store.MasterTenantID.String(), + Name: "Effective Skill", + Slug: "effective-skill", + Path: filepath.Join(root, "skills-store", "effective-skill", "1", "SKILL.md"), + BaseDir: filepath.Join(root, "skills-store", "effective-skill", "1"), + Visibility: "internal", + Status: "active", + Enabled: true, + } + skillStore.agentGrants[skillID] = []store.SkillAgentGrantInfo{{ + AgentID: agentID, + PinnedVersion: 2, + GrantedBy: "owner-user", + CanManage: true, + }} + + req := httptest.NewRequest(http.MethodGet, "/v1/skills/"+skillID.String()+"/access/effective?agent_id="+agentID.String()+"&user_id=target-user", http.NoBody).WithContext(ctx) + req.SetPathValue("id", skillID.String()) + w := httptest.NewRecorder() + + handler.handleGetSkillEffectiveAccess(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + var resp struct { + Accessible bool `json:"accessible"` + Reason string `json:"reason"` + CanManage bool `json:"can_manage"` + PinnedVersion *int `json:"pinned_version"` + } + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if !resp.Accessible || resp.Reason != "agent_grant" || !resp.CanManage || resp.PinnedVersion == nil || *resp.PinnedVersion != 2 { + t.Fatalf("effective access = %+v", resp) + } +} + +func TestEffectiveAccessTreatsAccessibleIndexAsSourceOfTruthForSystemSkills(t *testing.T) { + ctx := store.WithTenantID(context.Background(), uuid.New()) + sk := store.SkillInfo{ + ID: uuid.NewString(), + Name: "System Skill", + Slug: "system-skill", + Status: "active", + Enabled: true, + IsSystem: true, + } + resp := effectiveAccessForSkill(ctx, sk, effectiveAccessIndex{accessibleBySlug: map[string]bool{}}, "target-user") + if resp.Accessible || resp.Reason != "none" { + t.Fatalf("effective access = %+v, want inaccessible none", resp) + } + resp = effectiveAccessForSkill(ctx, sk, effectiveAccessIndex{accessibleBySlug: map[string]bool{"system-skill": true}}, "target-user") + if !resp.Accessible || resp.Reason != "system" { + t.Fatalf("effective access = %+v, want system access", resp) + } +} + +func TestHandlePatchSkillAccessRejectsNonMasterSystemSkillUpdate(t *testing.T) { + handler, skillStore, _, root := newTestUploadHandler(t) + tenantID := uuid.New() + skillID := skillStore.seedSystemSkill("system-skill", filepath.Join(root, "bundled", "system-skill")) + setupTestCache(t, map[string]*store.APIKeyData{ + crypto.HashAPIKey("tenant-admin-token"): { + ID: uuid.New(), + Scopes: []string{"operator.admin"}, + TenantID: tenantID, + OwnerID: "tenant-admin", + }, + }) + + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + req := httptest.NewRequest(http.MethodPatch, "/v1/skills/"+skillID.String()+"/access", strings.NewReader(`{"mode":"public"}`)) + req.Header.Set("Authorization", "Bearer tenant-admin-token") + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + + mux.ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body = %s", w.Code, w.Body.String()) + } + if updates := skillStore.lastUpdates[skillID]; updates != nil { + t.Fatalf("system skill updated by non-master tenant: %+v", updates) + } +} + +func TestHandlePatchSkillAccessAcceptsModeAlias(t *testing.T) { + handler, skillStore, ctx, root := newTestUploadHandler(t) + skillID := skillStore.seedCustomSkill("access-skill", filepath.Join(root, "skills-store", "access-skill", "1"), "active", nil) + + req := httptest.NewRequest(http.MethodPatch, "/v1/skills/"+skillID.String()+"/access", bytes.NewBufferString(`{"mode":"PUBLIC"}`)).WithContext(ctx) + req.Header.Set("Content-Type", "application/json") + req.SetPathValue("id", skillID.String()) + w := httptest.NewRecorder() + + handler.handlePatchSkillAccess(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + info, _ := skillStore.GetSkillByID(ctx, skillID) + if info.Visibility != "public" { + t.Fatalf("visibility = %q, want public", info.Visibility) + } +} diff --git a/internal/http/skills_upload_test.go b/internal/http/skills_upload_test.go index de45592c..0974784d 100644 --- a/internal/http/skills_upload_test.go +++ b/internal/http/skills_upload_test.go @@ -857,15 +857,18 @@ func skillMarkdown(name, slug string) string { } type skillManageStoreStub struct { - baseDir string - version int64 - nextBySlug map[string]int - skills map[uuid.UUID]store.SkillInfo - systemDirs map[string]string - hashBySlug map[string]string // slug -> SKILL.md content hash (most recent) - grantCalls []skillGrantCall - grantErrors map[uuid.UUID]error - lastUpdates map[uuid.UUID]map[string]any + baseDir string + version int64 + nextBySlug map[string]int + skills map[uuid.UUID]store.SkillInfo + systemDirs map[string]string + hashBySlug map[string]string // slug -> SKILL.md content hash (most recent) + grantCalls []skillGrantCall + userGrantCalls []skillUserGrantCall + grantErrors map[uuid.UUID]error + lastUpdates map[uuid.UUID]map[string]any + agentGrants map[uuid.UUID][]store.SkillAgentGrantInfo + userGrants map[uuid.UUID][]store.SkillUserGrantInfo } type skillUploadSystemConfigStore struct { @@ -904,6 +907,12 @@ type skillGrantCall struct { CanManage bool } +type skillUserGrantCall struct { + SkillID uuid.UUID + UserID string + GrantedBy string +} + func newSkillManageStoreStub(baseDir string) *skillManageStoreStub { return &skillManageStoreStub{ baseDir: baseDir, @@ -913,10 +922,12 @@ func newSkillManageStoreStub(baseDir string) *skillManageStoreStub { hashBySlug: map[string]string{}, grantErrors: map[uuid.UUID]error{}, lastUpdates: map[uuid.UUID]map[string]any{}, + agentGrants: map[uuid.UUID][]store.SkillAgentGrantInfo{}, + userGrants: map[uuid.UUID][]store.SkillUserGrantInfo{}, } } -func (s *skillManageStoreStub) seedSystemSkill(slug, dir string) { +func (s *skillManageStoreStub) seedSystemSkill(slug, dir string) uuid.UUID { id := uuid.New() s.skills[id] = store.SkillInfo{ ID: id.String(), @@ -931,6 +942,7 @@ func (s *skillManageStoreStub) seedSystemSkill(slug, dir string) { IsSystem: true, } s.systemDirs[slug] = dir + return id } func (s *skillManageStoreStub) seedCustomSkill(slug, dir, status string, missing []string) uuid.UUID { @@ -1143,15 +1155,89 @@ func (s *skillManageStoreStub) GrantToAgent(_ context.Context, skillID uuid.UUID func (s *skillManageStoreStub) RevokeFromAgent(context.Context, uuid.UUID, uuid.UUID) error { return nil } -func (s *skillManageStoreStub) GrantToUser(context.Context, uuid.UUID, string, string) error { +func (s *skillManageStoreStub) GrantToUser(_ context.Context, skillID uuid.UUID, userID, grantedBy string) error { + s.userGrantCalls = append(s.userGrantCalls, skillUserGrantCall{SkillID: skillID, UserID: userID, GrantedBy: grantedBy}) return nil } func (s *skillManageStoreStub) RevokeFromUser(context.Context, uuid.UUID, string) error { return nil } -func (s *skillManageStoreStub) ListWithGrantStatus(context.Context, uuid.UUID) ([]store.SkillWithGrantStatus, error) { - return nil, nil +func (s *skillManageStoreStub) ListAccessible(ctx context.Context, agentID uuid.UUID, userID string) ([]store.SkillInfo, error) { + actorID := store.ActorIDFromContext(ctx) + if actorID == "" { + actorID = userID + } + out := make([]store.SkillInfo, 0, len(s.skills)) + for id, skill := range s.skills { + if skill.Status != "active" || !skill.Enabled || !s.canAccessSkill(ctx, skill) { + continue + } + switch skill.Visibility { + case skills.VisibilityPublic: + out = append(out, skill) + case skills.VisibilityPrivate: + if skill.OwnerID == userID || skill.OwnerID == actorID { + out = append(out, skill) + } + case skills.VisibilityInternal: + if s.hasAgentGrant(id, agentID) || s.hasUserGrant(id, userID) || s.hasUserGrant(id, actorID) { + out = append(out, skill) + } + default: + if skill.IsSystem { + out = append(out, skill) + } + } + } + return out, nil } -func (s *skillManageStoreStub) ListAgentGrantsForSkill(context.Context, uuid.UUID) ([]store.SkillAgentGrantInfo, error) { - return nil, nil +func (s *skillManageStoreStub) ListWithGrantStatus(ctx context.Context, agentID uuid.UUID) ([]store.SkillWithGrantStatus, error) { + out := make([]store.SkillWithGrantStatus, 0, len(s.skills)) + for id, skill := range s.skills { + if skill.Status != "active" || !s.canAccessSkill(ctx, skill) { + continue + } + row := store.SkillWithGrantStatus{ + ID: id, + Name: skill.Name, + Slug: skill.Slug, + Description: skill.Description, + Visibility: skill.Visibility, + Version: skill.Version, + IsSystem: skill.IsSystem, + } + for _, grant := range s.agentGrants[id] { + if grant.AgentID == agentID { + row.Granted = true + row.CanManage = grant.CanManage + pinned := grant.PinnedVersion + row.PinnedVer = &pinned + break + } + } + out = append(out, row) + } + return out, nil +} +func (s *skillManageStoreStub) hasAgentGrant(skillID, agentID uuid.UUID) bool { + for _, grant := range s.agentGrants[skillID] { + if grant.AgentID == agentID { + return true + } + } + return false +} +func (s *skillManageStoreStub) hasUserGrant(skillID uuid.UUID, userID string) bool { + for _, grant := range s.userGrants[skillID] { + if grant.UserID == userID { + return true + } + } + return false +} +func (s *skillManageStoreStub) ListAgentGrantsForSkill(_ context.Context, skillID uuid.UUID) ([]store.SkillAgentGrantInfo, error) { + return append([]store.SkillAgentGrantInfo(nil), s.agentGrants[skillID]...), nil +} +func (s *skillManageStoreStub) ListUserGrantsForSkill(_ context.Context, skillID uuid.UUID) ([]store.SkillUserGrantInfo, error) { + return append([]store.SkillUserGrantInfo(nil), s.userGrants[skillID]...), nil } func (s *skillManageStoreStub) AgentCanManageSkill(context.Context, uuid.UUID, uuid.UUID) (bool, error) { return false, nil diff --git a/internal/skills/dep_installer.go b/internal/skills/dep_installer.go index 0628b0ba..528b0216 100644 --- a/internal/skills/dep_installer.go +++ b/internal/skills/dep_installer.go @@ -50,6 +50,7 @@ type InstallResult struct { System []string `json:"system,omitempty"` Pip []string `json:"pip,omitempty"` Npm []string `json:"npm,omitempty"` + GitHub []string `json:"github,omitempty"` Errors []string `json:"errors,omitempty"` } diff --git a/internal/skills/dep_scanner.go b/internal/skills/dep_scanner.go index e828ab89..c0b10e39 100644 --- a/internal/skills/dep_scanner.go +++ b/internal/skills/dep_scanner.go @@ -24,7 +24,7 @@ type SkillManifest struct { // IsEmpty returns true if the manifest has no dependencies. func (m *SkillManifest) IsEmpty() bool { - return len(m.Requires) == 0 && len(m.RequiresPython) == 0 && len(m.RequiresNode) == 0 + return len(m.Requires) == 0 && len(m.RequiresPython) == 0 && len(m.RequiresNode) == 0 && len(m.Explicit) == 0 } // ScanSkillDeps auto-detects dependencies by statically analyzing the scripts/ directory, diff --git a/internal/store/pg/skills_grants.go b/internal/store/pg/skills_grants.go index 04a710a4..8374fe20 100644 --- a/internal/store/pg/skills_grants.go +++ b/internal/store/pg/skills_grants.go @@ -321,17 +321,37 @@ func (s *PGSkillStore) GrantToUser(ctx context.Context, skillID uuid.UUID, userI if err := store.ValidateUserID(grantedBy); err != nil { return err } + tid := tenantIDForInsert(ctx) + if err := s.verifySkillInGrantScope(ctx, skillID, tid); err != nil { + return err + } _, err := s.db.ExecContext(ctx, `INSERT INTO skill_user_grants (id, skill_id, user_id, granted_by, created_at, tenant_id) VALUES ($1, $2, $3, $4, $5, $6) - ON CONFLICT (skill_id, user_id) DO NOTHING`, - store.GenNewID(), skillID, userID, grantedBy, time.Now(), tenantIDForInsert(ctx), + ON CONFLICT (skill_id, user_id, tenant_id) DO NOTHING`, + store.GenNewID(), skillID, userID, grantedBy, time.Now(), tid, ) - return err + if err != nil { + return err + } + _, err = s.db.ExecContext(ctx, + `UPDATE skills + SET visibility = 'internal', updated_at = NOW() + WHERE id = $1 AND visibility = 'private' AND (is_system = true OR tenant_id = $2)`, + skillID, tid) + if err != nil { + slog.Warn("skill_grants: failed to auto-promote visibility for user grant", "skill_id", skillID, "error", err) + } + s.BumpVersion() + return nil } // RevokeFromUser revokes a skill grant from a user. func (s *PGSkillStore) RevokeFromUser(ctx context.Context, skillID uuid.UUID, userID string) error { + tid := tenantIDForInsert(ctx) + if err := s.verifySkillInGrantScope(ctx, skillID, tid); err != nil { + return err + } tClause, tArgs, _, err := scopeClause(ctx, 3) if err != nil { return err @@ -339,7 +359,42 @@ func (s *PGSkillStore) RevokeFromUser(ctx context.Context, skillID uuid.UUID, us _, err = s.db.ExecContext(ctx, "DELETE FROM skill_user_grants WHERE skill_id = $1 AND user_id = $2"+tClause, append([]any{skillID, userID}, tArgs...)...) - return err + if err != nil { + return err + } + _, err = s.db.ExecContext(ctx, + `UPDATE skills SET visibility = 'private', updated_at = NOW() + WHERE id = $1 AND visibility = 'internal' AND (is_system = true OR tenant_id = $2) + AND NOT EXISTS (SELECT 1 FROM skill_agent_grants WHERE skill_id = $1) + AND NOT EXISTS (SELECT 1 FROM skill_user_grants WHERE skill_id = $1)`, + skillID, tid) + if err != nil { + slog.Warn("skill_grants: failed to auto-demote visibility for user grant", "skill_id", skillID, "error", err) + } + s.BumpVersion() + return nil +} + +// ListUserGrantsForSkill returns all user grants for one skill. +func (s *PGSkillStore) ListUserGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]store.SkillUserGrantInfo, error) { + if err := s.verifySkillInGrantScope(ctx, skillID, tenantIDForInsert(ctx)); err != nil { + return nil, err + } + tClause, tArgs, _, err := scopeClauseAlias(ctx, 2, "sug") + if err != nil { + return nil, err + } + var result []store.SkillUserGrantInfo + err = pkgSqlxDB.SelectContext(ctx, &result, + `SELECT sug.user_id, sug.granted_by + FROM skill_user_grants sug + WHERE sug.skill_id = $1`+tClause+` + ORDER BY sug.created_at DESC`, + append([]any{skillID}, tArgs...)...) + if err != nil { + return nil, err + } + return result, nil } // ListAccessible returns skills accessible to a given agent+user combination. @@ -364,9 +419,13 @@ func (s *PGSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, us return nil, err } tenantCond := "" + agentGrantTenantCond := "" + userGrantTenantCond := "" if tc != "" { // tc is " AND tenant_id = $4"; we need it as an OR condition inside the WHERE tenantCond = fmt.Sprintf(" AND (s.is_system = true OR s.tenant_id = $%d)", 4) + agentGrantTenantCond = fmt.Sprintf(" AND sag.tenant_id = $%d", 4) + userGrantTenantCond = fmt.Sprintf(" AND sug.tenant_id = $%d", 4) _ = tc // tcArgs carries the value } // LEFT JOIN skill_tenant_configs to exclude per-tenant disabled skills. @@ -379,8 +438,8 @@ func (s *PGSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, us } rows, err := s.db.QueryContext(ctx, `SELECT DISTINCT s.name, s.slug, s.description, s.version, s.file_path FROM skills s - LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = $1 - LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND (sug.user_id = $2 OR sug.user_id = $3)`+stcJoin+` + LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = $1`+agentGrantTenantCond+` + LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND (sug.user_id = $2 OR sug.user_id = $3)`+userGrantTenantCond+stcJoin+` WHERE s.status = 'active'`+tenantCond+stcFilter+` AND ( s.is_system = true OR s.visibility = 'public' diff --git a/internal/store/skill_store.go b/internal/store/skill_store.go index 7ac897fd..5d554d80 100644 --- a/internal/store/skill_store.go +++ b/internal/store/skill_store.go @@ -114,6 +114,12 @@ type SkillAgentGrantInfo struct { CanManage bool `json:"can_manage" db:"can_manage"` } +// SkillUserGrantInfo is a user grant row for one skill. +type SkillUserGrantInfo struct { + UserID string `json:"user_id" db:"user_id"` + GrantedBy string `json:"granted_by" db:"granted_by"` +} + // SkillManageStore extends SkillStore with CRUD, ownership, and grant operations // needed by HTTP upload handlers and agent tools (skill_manage, publish_skill). // Implemented by both PGSkillStore and SQLiteSkillStore. @@ -146,6 +152,7 @@ type SkillManageStore interface { RevokeFromUser(ctx context.Context, skillID uuid.UUID, userID string) error ListWithGrantStatus(ctx context.Context, agentID uuid.UUID) ([]SkillWithGrantStatus, error) ListAgentGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]SkillAgentGrantInfo, error) + ListUserGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]SkillUserGrantInfo, error) AgentCanManageSkill(ctx context.Context, skillID, agentID uuid.UUID) (bool, error) // Files GetSkillFilePath(ctx context.Context, id uuid.UUID) (filePath string, slug string, version int, isSystem bool, ok bool) diff --git a/internal/store/sqlitestore/schema.go b/internal/store/sqlitestore/schema.go index e7791b79..af8816e0 100644 --- a/internal/store/sqlitestore/schema.go +++ b/internal/store/sqlitestore/schema.go @@ -16,7 +16,7 @@ var schemaSQL string // SchemaVersion is the current SQLite schema version. // Bump this when adding new migration steps below. -const SchemaVersion = 46 +const SchemaVersion = 47 // migrations maps version → SQL to apply when upgrading FROM that version. // schema.sql always represents the LATEST full schema (for fresh DBs). @@ -831,6 +831,23 @@ CREATE TABLE IF NOT EXISTS secure_cli_agent_credentials ( CREATE INDEX IF NOT EXISTS idx_scac_tenant ON secure_cli_agent_credentials(tenant_id); CREATE INDEX IF NOT EXISTS idx_scac_binary ON secure_cli_agent_credentials(binary_id); CREATE INDEX IF NOT EXISTS idx_scac_agent ON secure_cli_agent_credentials(agent_id);`, + // Version 46 → 47: scope skill user-grant uniqueness by tenant. + 46: `CREATE TABLE skill_user_grants_new ( + id TEXT NOT NULL PRIMARY KEY, + skill_id TEXT NOT NULL REFERENCES skills(id) ON DELETE CASCADE, + user_id VARCHAR(255) NOT NULL, + granted_by VARCHAR(255) NOT NULL, + tenant_id TEXT NOT NULL REFERENCES tenants(id), + created_at TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), + UNIQUE(skill_id, user_id, tenant_id) +); +INSERT OR IGNORE INTO skill_user_grants_new (id, skill_id, user_id, granted_by, tenant_id, created_at) + SELECT id, skill_id, user_id, granted_by, tenant_id, created_at + FROM skill_user_grants; +DROP TABLE skill_user_grants; +ALTER TABLE skill_user_grants_new RENAME TO skill_user_grants; +CREATE INDEX IF NOT EXISTS idx_skill_user_grants_user ON skill_user_grants(user_id); +CREATE INDEX IF NOT EXISTS idx_skill_user_grants_tenant ON skill_user_grants(tenant_id);`, } const addChannelMemoryExtractionTables = ` diff --git a/internal/store/sqlitestore/schema.sql b/internal/store/sqlitestore/schema.sql index 28989c07..df8eb703 100644 --- a/internal/store/sqlitestore/schema.sql +++ b/internal/store/sqlitestore/schema.sql @@ -428,7 +428,7 @@ CREATE TABLE IF NOT EXISTS skill_user_grants ( granted_by VARCHAR(255) NOT NULL, tenant_id TEXT NOT NULL REFERENCES tenants(id), created_at TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), - UNIQUE(skill_id, user_id) + UNIQUE(skill_id, user_id, tenant_id) ); CREATE INDEX IF NOT EXISTS idx_skill_user_grants_user ON skill_user_grants(user_id); diff --git a/internal/store/sqlitestore/skills.go b/internal/store/sqlitestore/skills.go index ff6d8182..62b89236 100644 --- a/internal/store/sqlitestore/skills.go +++ b/internal/store/sqlitestore/skills.go @@ -123,18 +123,18 @@ func (s *SQLiteSkillStore) ListAllSkills(ctx context.Context) []store.SkillInfo var err error if store.IsCrossTenant(ctx) { rows, err = s.db.QueryContext(ctx, - `SELECT id, tenant_id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, file_path - FROM skills WHERE enabled = 1 AND status != 'deleted' - ORDER BY name`) + `SELECT id, tenant_id, name, slug, description, visibility, owner_id, tags, version, is_system, status, enabled, deps, file_path + FROM skills WHERE enabled = 1 AND status != 'deleted' + ORDER BY name`) } else { tid := store.TenantIDFromContext(ctx) if tid == uuid.Nil { tid = store.MasterTenantID } rows, err = s.db.QueryContext(ctx, - `SELECT id, tenant_id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, file_path - FROM skills WHERE enabled = 1 AND status != 'deleted' AND (is_system = 1 OR tenant_id = ?) - ORDER BY name`, tid) + `SELECT id, tenant_id, name, slug, description, visibility, owner_id, tags, version, is_system, status, enabled, deps, file_path + FROM skills WHERE enabled = 1 AND status != 'deleted' AND (is_system = 1 OR tenant_id = ?) + ORDER BY name`, tid) } if err != nil { return nil @@ -145,9 +145,9 @@ func (s *SQLiteSkillStore) ListAllSkills(ctx context.Context) []store.SkillInfo func (s *SQLiteSkillStore) ListAllSystemSkills(ctx context.Context) []store.SkillInfo { rows, err := s.db.QueryContext(ctx, - `SELECT id, tenant_id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, file_path - FROM skills WHERE is_system = 1 AND enabled = 1 AND status != 'deleted' - ORDER BY name`) + `SELECT id, tenant_id, name, slug, description, visibility, owner_id, tags, version, is_system, status, enabled, deps, file_path + FROM skills WHERE is_system = 1 AND enabled = 1 AND status != 'deleted' + ORDER BY name`) if err != nil { return nil } @@ -160,20 +160,21 @@ func (s *SQLiteSkillStore) scanSkillInfoList(rows *sql.Rows) []store.SkillInfo { for rows.Next() { var id uuid.UUID var tenantID 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 []byte var filePath *string - if err := rows.Scan(&id, &tenantID, &name, &slug, &desc, &visibility, &tagsJSON, &version, + if err := rows.Scan(&id, &tenantID, &name, &slug, &desc, &visibility, &ownerID, &tagsJSON, &version, &isSystem, &status, &enabled, &depsRaw, &filePath); err != nil { continue } info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir, filePath) info.TenantID = tenantID.String() info.Visibility = visibility + info.OwnerID = ownerID scanJSONStringArray(tagsJSON, &info.Tags) info.IsSystem = isSystem info.Status = status diff --git a/internal/store/sqlitestore/skills_grants.go b/internal/store/sqlitestore/skills_grants.go index 823b5ff7..9385c5a7 100644 --- a/internal/store/sqlitestore/skills_grants.go +++ b/internal/store/sqlitestore/skills_grants.go @@ -370,18 +370,38 @@ func (s *SQLiteSkillStore) GrantToUser(ctx context.Context, skillID uuid.UUID, u if err := store.ValidateUserID(grantedBy); err != nil { return err } + tid := tenantIDForInsert(ctx) + if err := s.verifySkillInGrantScope(ctx, skillID, tid); err != nil { + return err + } id := store.GenNewID() _, err := s.db.ExecContext(ctx, `INSERT INTO skill_user_grants (id, skill_id, user_id, granted_by, created_at, tenant_id) VALUES (?, ?, ?, ?, ?, ?) - ON CONFLICT (skill_id, user_id) DO NOTHING`, - id, skillID, userID, grantedBy, time.Now().UTC(), tenantIDForInsert(ctx), + ON CONFLICT (skill_id, user_id, tenant_id) DO NOTHING`, + id, skillID, userID, grantedBy, time.Now().UTC(), tid, ) - return err + if err != nil { + return err + } + _, err = s.db.ExecContext(ctx, + `UPDATE skills + SET visibility = 'internal', updated_at = ? + WHERE id = ? AND visibility = 'private' AND (is_system = 1 OR tenant_id = ?)`, + time.Now().UTC(), skillID, tid) + if err != nil { + slog.Warn("skill_grants: failed to auto-promote visibility for user grant", "skill_id", skillID, "error", err) + } + s.BumpVersion() + return nil } // RevokeFromUser revokes a skill grant from a user. func (s *SQLiteSkillStore) RevokeFromUser(ctx context.Context, skillID uuid.UUID, userID string) error { + tid := tenantIDForInsert(ctx) + if err := s.verifySkillInGrantScope(ctx, skillID, tid); err != nil { + return err + } tClause, tArgs, err := scopeClause(ctx) if err != nil { return err @@ -389,7 +409,52 @@ func (s *SQLiteSkillStore) RevokeFromUser(ctx context.Context, skillID uuid.UUID _, err = s.db.ExecContext(ctx, "DELETE FROM skill_user_grants WHERE skill_id = ? AND user_id = ?"+tClause, append([]any{skillID, userID}, tArgs...)...) - return err + if err != nil { + return err + } + _, err = s.db.ExecContext(ctx, + `UPDATE skills SET visibility = 'private', updated_at = ? + WHERE id = ? AND visibility = 'internal' AND (is_system = 1 OR tenant_id = ?) + AND NOT EXISTS (SELECT 1 FROM skill_agent_grants WHERE skill_id = ?) + AND NOT EXISTS (SELECT 1 FROM skill_user_grants WHERE skill_id = ?)`, + time.Now().UTC(), skillID, tid, skillID, skillID) + if err != nil { + slog.Warn("skill_grants: failed to auto-demote visibility for user grant", "skill_id", skillID, "error", err) + } + s.BumpVersion() + return nil +} + +// ListUserGrantsForSkill returns all user grants for one skill. +func (s *SQLiteSkillStore) ListUserGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]store.SkillUserGrantInfo, error) { + if err := s.verifySkillInGrantScope(ctx, skillID, tenantIDForInsert(ctx)); err != nil { + return nil, err + } + tClause, tArgs, err := scopeClause(ctx) + if err != nil { + return nil, err + } + rows, err := s.db.QueryContext(ctx, + `SELECT sug.user_id, sug.granted_by + FROM skill_user_grants sug + WHERE sug.skill_id = ?`+strings.ReplaceAll(tClause, "tenant_id", "sug.tenant_id")+` + ORDER BY sug.created_at DESC`, + append([]any{skillID}, tArgs...)...) + if err != nil { + return nil, err + } + defer rows.Close() + + var result []store.SkillUserGrantInfo + for rows.Next() { + var g store.SkillUserGrantInfo + if err := rows.Scan(&g.UserID, &g.GrantedBy); err != nil { + slog.Warn("skill_grants: scan error in ListUserGrantsForSkill", "error", err) + continue + } + result = append(result, g) + } + return result, rows.Err() } // ListAccessible returns skills accessible to a given agent+user combination. @@ -404,27 +469,35 @@ func (s *SQLiteSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID return nil, err } tenantCond := "" + agentGrantTenantCond := "" + userGrantTenantCond := "" stcJoin := "" stcFilter := "" if len(tArgs) > 0 { tenantCond = " AND (s.is_system = 1 OR s.tenant_id = ?)" + agentGrantTenantCond = " AND sag.tenant_id = ?" + userGrantTenantCond = " AND sug.tenant_id = ?" stcJoin = " LEFT JOIN skill_tenant_configs stc ON s.id = stc.skill_id AND stc.tenant_id = ?" stcFilter = " AND (stc.enabled IS NULL OR stc.enabled = 1)" } - // Positional args: agentID, userID, actorID, [tenantID x2 if tenant-scoped], userID, actorID (private-owner clause) - queryArgs := []any{agentID, userID, actorID} + queryArgs := []any{agentID} if len(tArgs) > 0 { - queryArgs = append(queryArgs, tArgs...) // tenant cond + queryArgs = append(queryArgs, tArgs...) // sag tenant + } + queryArgs = append(queryArgs, userID, actorID) + if len(tArgs) > 0 { + queryArgs = append(queryArgs, tArgs...) // sug tenant queryArgs = append(queryArgs, tArgs...) // stc join + queryArgs = append(queryArgs, tArgs...) // tenant cond } // Remove tClause (aliased scope) — we handle it manually above. _ = tClause rows, err := s.db.QueryContext(ctx, `SELECT DISTINCT s.name, s.slug, s.description, s.version, s.file_path FROM skills s - LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = ? - LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND (sug.user_id = ? OR sug.user_id = ?)`+stcJoin+` + LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = ?`+agentGrantTenantCond+` + LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND (sug.user_id = ? OR sug.user_id = ?)`+userGrantTenantCond+stcJoin+` WHERE s.status = 'active'`+tenantCond+stcFilter+` AND ( s.is_system = 1 OR s.visibility = 'public' diff --git a/internal/store/sqlitestore/skills_test.go b/internal/store/sqlitestore/skills_test.go index 76fcf03c..93be73cf 100644 --- a/internal/store/sqlitestore/skills_test.go +++ b/internal/store/sqlitestore/skills_test.go @@ -250,6 +250,89 @@ func TestSQLiteSkillStore_RevokeFromAgentKeepsInternalWhenUserGrantRemains(t *te } } +func TestSQLiteSkillStore_UserGrantListPromotesAndDemotesVisibility(t *testing.T) { + _, skillStore, db := newTestSQLiteSkillStoreWithDB(t) + tenantID, _ := seedSQLiteTenantAgent(t, db) + ctx := store.WithTenantID(context.Background(), tenantID) + + skillID, err := skillStore.CreateSkillManaged(ctx, store.SkillCreateParams{ + Name: "User Shared Skill", + Slug: "user-shared-skill-" + tenantID.String()[:8], + OwnerID: "owner-user", + Visibility: "private", + FilePath: filepath.Join(t.TempDir(), "user-shared-skill", "1"), + }) + if err != nil { + t.Fatalf("CreateSkillManaged error: %v", err) + } + if err := skillStore.GrantToUser(ctx, skillID, "granted-user", "owner-user"); err != nil { + t.Fatalf("GrantToUser error: %v", err) + } + + grants, err := skillStore.ListUserGrantsForSkill(ctx, skillID) + if err != nil { + t.Fatalf("ListUserGrantsForSkill error: %v", err) + } + if len(grants) != 1 || grants[0].UserID != "granted-user" || grants[0].GrantedBy != "owner-user" { + t.Fatalf("user grants = %+v", grants) + } + got, ok := skillStore.GetSkillByID(ctx, skillID) + if !ok { + t.Fatal("GetSkillByID returned !ok") + } + if got.Visibility != "internal" { + t.Fatalf("visibility after user grant = %q, want internal", got.Visibility) + } + if err := skillStore.RevokeFromUser(ctx, skillID, "granted-user"); err != nil { + t.Fatalf("RevokeFromUser error: %v", err) + } + got, ok = skillStore.GetSkillByID(ctx, skillID) + if !ok { + t.Fatal("GetSkillByID returned !ok after revoke") + } + if got.Visibility != "private" { + t.Fatalf("visibility after last user grant revoke = %q, want private", got.Visibility) + } +} + +func TestSQLiteSkillStore_UserGrantsAreTenantScopedForSystemSkill(t *testing.T) { + _, skillStore, db := newTestSQLiteSkillStoreWithDB(t) + tenantA, _ := seedSQLiteTenantAgent(t, db) + tenantB, _ := seedSQLiteTenantAgent(t, db) + ctxA := store.WithTenantID(context.Background(), tenantA) + ctxB := store.WithTenantID(context.Background(), tenantB) + skillID := uuid.New() + slug := "system-user-grant-" + skillID.String()[:8] + if _, err := db.Exec( + `INSERT INTO skills (id, name, slug, owner_id, visibility, version, status, file_path, is_system, tenant_id) + VALUES (?, 'System User Grant Skill', ?, 'system', 'private', 1, 'active', ?, 1, ?)`, + skillID.String(), slug, filepath.Join(t.TempDir(), "system-user-grant", "1"), store.MasterTenantID.String(), + ); err != nil { + t.Fatalf("insert system skill: %v", err) + } + + if err := skillStore.GrantToUser(ctxA, skillID, "same-user", "tenant-a-admin"); err != nil { + t.Fatalf("GrantToUser tenant A error: %v", err) + } + if err := skillStore.GrantToUser(ctxB, skillID, "same-user", "tenant-b-admin"); err != nil { + t.Fatalf("GrantToUser tenant B error: %v", err) + } + grantsA, err := skillStore.ListUserGrantsForSkill(ctxA, skillID) + if err != nil { + t.Fatalf("ListUserGrantsForSkill tenant A error: %v", err) + } + grantsB, err := skillStore.ListUserGrantsForSkill(ctxB, skillID) + if err != nil { + t.Fatalf("ListUserGrantsForSkill tenant B error: %v", err) + } + if len(grantsA) != 1 || grantsA[0].GrantedBy != "tenant-a-admin" { + t.Fatalf("tenant A grants = %+v", grantsA) + } + if len(grantsB) != 1 || grantsB[0].GrantedBy != "tenant-b-admin" { + t.Fatalf("tenant B grants = %+v", grantsB) + } +} + func TestSQLiteSkillStore_ListWithGrantStatusIgnoresForeignTenantGrant(t *testing.T) { _, skillStore, db := newTestSQLiteSkillStoreWithDB(t) tenantA, _ := seedSQLiteTenantAgent(t, db) @@ -365,6 +448,30 @@ func TestSQLiteSkillStore_ListAccessibleHonorsAccessModes(t *testing.T) { } } +func TestSQLiteSkillStore_ListAllSkillsIncludesOwnerID(t *testing.T) { + ctx, skillStore := newTestSQLiteSkillStore(t) + skillID, err := skillStore.CreateSkillManaged(ctx, store.SkillCreateParams{ + Name: "Owner Projection Skill", + Slug: "owner-projection-skill", + OwnerID: "owner-user", + Visibility: "private", + FilePath: filepath.Join(t.TempDir(), "owner-projection", "1"), + }) + if err != nil { + t.Fatalf("CreateSkillManaged error: %v", err) + } + + for _, skill := range skillStore.ListAllSkills(ctx) { + if skill.ID == skillID.String() { + if skill.OwnerID != "owner-user" { + t.Fatalf("OwnerID = %q, want owner-user", skill.OwnerID) + } + return + } + } + t.Fatalf("created skill %s not found", skillID) +} + func listAccessibleSlugs(t *testing.T, skillStore *SQLiteSkillStore, ctx context.Context, agentID uuid.UUID, userID string) map[string]bool { t.Helper() skills, err := skillStore.ListAccessible(ctx, agentID, userID) diff --git a/internal/tools/skill_manage_files_test.go b/internal/tools/skill_manage_files_test.go index 298df992..4e7fa96d 100644 --- a/internal/tools/skill_manage_files_test.go +++ b/internal/tools/skill_manage_files_test.go @@ -594,6 +594,9 @@ func (s *skillManageFilesStore) ListWithGrantStatus(context.Context, uuid.UUID) func (s *skillManageFilesStore) ListAgentGrantsForSkill(context.Context, uuid.UUID) ([]store.SkillAgentGrantInfo, error) { return nil, nil } +func (s *skillManageFilesStore) ListUserGrantsForSkill(context.Context, uuid.UUID) ([]store.SkillUserGrantInfo, error) { + return nil, nil +} func (s *skillManageFilesStore) AgentCanManageSkill(context.Context, uuid.UUID, uuid.UUID) (bool, error) { return false, nil } diff --git a/internal/upgrade/version.go b/internal/upgrade/version.go index 36f249f9..67b4ce6e 100644 --- a/internal/upgrade/version.go +++ b/internal/upgrade/version.go @@ -2,4 +2,4 @@ package upgrade // RequiredSchemaVersion is the schema migration version this binary requires. // Bump this whenever adding a new SQL migration file. -const RequiredSchemaVersion uint = 77 +const RequiredSchemaVersion uint = 78 diff --git a/migrations/000078_skill_user_grants_tenant_unique.down.sql b/migrations/000078_skill_user_grants_tenant_unique.down.sql new file mode 100644 index 00000000..d1a8bb95 --- /dev/null +++ b/migrations/000078_skill_user_grants_tenant_unique.down.sql @@ -0,0 +1,6 @@ +ALTER TABLE skill_user_grants + DROP CONSTRAINT IF EXISTS skill_user_grants_skill_id_user_id_tenant_id_key; + +ALTER TABLE skill_user_grants + ADD CONSTRAINT skill_user_grants_skill_id_user_id_key + UNIQUE (skill_id, user_id); diff --git a/migrations/000078_skill_user_grants_tenant_unique.up.sql b/migrations/000078_skill_user_grants_tenant_unique.up.sql new file mode 100644 index 00000000..936a749a --- /dev/null +++ b/migrations/000078_skill_user_grants_tenant_unique.up.sql @@ -0,0 +1,9 @@ +ALTER TABLE skill_user_grants + DROP CONSTRAINT IF EXISTS skill_user_grants_skill_id_user_id_key; + +ALTER TABLE skill_user_grants + DROP CONSTRAINT IF EXISTS skill_user_grants_skill_id_user_id_tenant_id_key; + +ALTER TABLE skill_user_grants + ADD CONSTRAINT skill_user_grants_skill_id_user_id_tenant_id_key + UNIQUE (skill_id, user_id, tenant_id); diff --git a/tests/integration/v3_skills_store_test.go b/tests/integration/v3_skills_store_test.go index 0ae48a34..1bb75fb1 100644 --- a/tests/integration/v3_skills_store_test.go +++ b/tests/integration/v3_skills_store_test.go @@ -346,6 +346,81 @@ func TestStoreSkill_GrantToAgent(t *testing.T) { } } +func TestStoreSkill_UserGrantListPromotesAndDemotesVisibility(t *testing.T) { + db := testDB(t) + tenantID, _ := seedTenantAgent(t, db) + ctx := tenantCtx(tenantID) + s := newSkillStore(t) + + skillID := seedSkill(t, s, ctx, "user-grant-skill-"+tenantID.String()[:8], "User Grant Skill") + if err := s.GrantToUser(ctx, skillID, "granted-user", "test-owner"); err != nil { + t.Fatalf("GrantToUser: %v", err) + } + grants, err := s.ListUserGrantsForSkill(ctx, skillID) + if err != nil { + t.Fatalf("ListUserGrantsForSkill: %v", err) + } + if len(grants) != 1 || grants[0].UserID != "granted-user" || grants[0].GrantedBy != "test-owner" { + t.Fatalf("user grants = %+v", grants) + } + got, ok := s.GetSkillByID(ctx, skillID) + if !ok { + t.Fatal("GetSkillByID returned false") + } + if got.Visibility != "internal" { + t.Fatalf("visibility after user grant = %q, want internal", got.Visibility) + } + if err := s.RevokeFromUser(ctx, skillID, "granted-user"); err != nil { + t.Fatalf("RevokeFromUser: %v", err) + } + got, ok = s.GetSkillByID(ctx, skillID) + if !ok { + t.Fatal("GetSkillByID returned false after revoke") + } + if got.Visibility != "private" { + t.Fatalf("visibility after last user grant revoke = %q, want private", got.Visibility) + } +} + +func TestStoreSkill_UserGrantsAreTenantScopedForSystemSkill(t *testing.T) { + db := testDB(t) + tenantA, _ := seedTenantAgent(t, db) + tenantB, _ := seedTenantAgent(t, db) + ctxA := tenantCtx(tenantA) + ctxB := tenantCtx(tenantB) + s := newSkillStore(t) + skillID := uuid.New() + slug := "system-user-grant-" + skillID.String()[:8] + if _, err := db.Exec( + `INSERT INTO skills (id, name, slug, owner_id, visibility, version, status, file_path, is_system, tenant_id) + VALUES ($1, 'System User Grant Skill', $2, 'system', 'private', 1, 'active', $3, true, $4)`, + skillID, slug, "/tmp/skills/system-user-grant/1", store.MasterTenantID, + ); err != nil { + t.Fatalf("insert system skill: %v", err) + } + + if err := s.GrantToUser(ctxA, skillID, "same-user", "tenant-a-admin"); err != nil { + t.Fatalf("GrantToUser tenant A: %v", err) + } + if err := s.GrantToUser(ctxB, skillID, "same-user", "tenant-b-admin"); err != nil { + t.Fatalf("GrantToUser tenant B: %v", err) + } + grantsA, err := s.ListUserGrantsForSkill(ctxA, skillID) + if err != nil { + t.Fatalf("ListUserGrantsForSkill tenant A: %v", err) + } + grantsB, err := s.ListUserGrantsForSkill(ctxB, skillID) + if err != nil { + t.Fatalf("ListUserGrantsForSkill tenant B: %v", err) + } + if len(grantsA) != 1 || grantsA[0].GrantedBy != "tenant-a-admin" { + t.Fatalf("tenant A grants = %+v", grantsA) + } + if len(grantsB) != 1 || grantsB[0].GrantedBy != "tenant-b-admin" { + t.Fatalf("tenant B grants = %+v", grantsB) + } +} + func TestStoreSkill_GrantToAgentRejectsCrossTenantSkill(t *testing.T) { db := testDB(t) tenantA, agentA := seedTenantAgent(t, db)