From 2ac8c3f5fb722d3e02e6b0f49cd837566670e554 Mon Sep 17 00:00:00 2001 From: lxowalle Date: Mon, 13 Apr 2026 18:19:08 +0800 Subject: [PATCH] fix skills config compatibility and legacy security overlays --- cmd/picoclaw/internal/skills/command.go | 2 +- cmd/picoclaw/internal/skills/helpers.go | 12 ++- cmd/picoclaw/internal/skills/helpers_test.go | 92 +++++++++++++++++++ cmd/picoclaw/internal/skills/remove.go | 8 +- cmd/picoclaw/internal/skills/remove_test.go | 2 +- pkg/agent/hooks_test.go | 25 ++++- pkg/config/config.go | 54 ++++++++--- pkg/config/config_struct.go | 12 ++- pkg/config/config_struct_test.go | 41 +++++++++ pkg/config/config_test.go | 36 ++++++++ pkg/config/security.go | 96 ++++++++++++++++++++ pkg/config/security_integration_test.go | 73 +++++++++++++++ pkg/skills/config_bridge.go | 9 ++ pkg/skills/github_registry.go | 9 +- pkg/skills/github_registry_test.go | 1 + pkg/skills/installer.go | 45 +++++---- pkg/skills/installer_test.go | 36 +++++++- web/backend/api/config_test.go | 52 +++++++++++ web/backend/api/pico_test.go | 6 +- web/backend/api/skills.go | 9 +- web/backend/api/skills_test.go | 59 ++++++++++++ 21 files changed, 623 insertions(+), 56 deletions(-) diff --git a/cmd/picoclaw/internal/skills/command.go b/cmd/picoclaw/internal/skills/command.go index 678031c0a..151605264 100644 --- a/cmd/picoclaw/internal/skills/command.go +++ b/cmd/picoclaw/internal/skills/command.go @@ -61,7 +61,7 @@ func NewSkillsCommand() *cobra.Command { newInstallCommand(), newInstallBuiltinCommand(workspaceFn), newListBuiltinCommand(), - newRemoveCommand(workspaceFn), + newRemoveCommand(), newSearchCommand(), newShowCommand(loaderFn), ) diff --git a/cmd/picoclaw/internal/skills/helpers.go b/cmd/picoclaw/internal/skills/helpers.go index af552d22a..e27a32711 100644 --- a/cmd/picoclaw/internal/skills/helpers.go +++ b/cmd/picoclaw/internal/skills/helpers.go @@ -153,15 +153,21 @@ func workspaceHasValidSkillDirectory(workspace, directory string) bool { return false } -func skillsRemoveFromWorkspace(workspace, skillName string) error { +func skillsRemoveFromWorkspace(workspace string, toolsConfig config.SkillsToolsConfig, skillName string) error { name := strings.TrimSpace(skillName) name = strings.Trim(name, "/") if name == "" { return fmt.Errorf("skill name is required") } if strings.Contains(name, "/") { - parts := strings.Split(name, "/") - name = parts[len(parts)-1] + dirName, err := skills.GitHubInstallDirNameFromToolsConfig(toolsConfig, name) + if err != nil || dirName == "" { + return fmt.Errorf("invalid skill name %q", skillName) + } + name = dirName + } + if name == "." || name == ".." { + return fmt.Errorf("invalid skill name %q", skillName) } skillDir := filepath.Join(workspace, "skills", name) if _, err := os.Stat(skillDir); os.IsNotExist(err) { diff --git a/cmd/picoclaw/internal/skills/helpers_test.go b/cmd/picoclaw/internal/skills/helpers_test.go index 215cd9af9..366b7f8a8 100644 --- a/cmd/picoclaw/internal/skills/helpers_test.go +++ b/cmd/picoclaw/internal/skills/helpers_test.go @@ -97,3 +97,95 @@ func TestSkillsInstallFromRegistryRejectsInvalidSkillArchive(t *testing.T) { _, statErr := os.Stat(filepath.Join(workspace, "skills", "pr-review")) assert.True(t, os.IsNotExist(statErr)) } + +func TestSkillsRemoveFromWorkspaceRejectsDotTarget(t *testing.T) { + workspace := t.TempDir() + skillsDir := filepath.Join(workspace, "skills") + require.NoError(t, os.MkdirAll(skillsDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillsDir, "keep.txt"), []byte("keep"), 0o644)) + + err := skillsRemoveFromWorkspace(workspace, config.DefaultConfig().Tools.Skills, ".") + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid skill name") + + _, statErr := os.Stat(skillsDir) + assert.NoError(t, statErr) + _, fileErr := os.Stat(filepath.Join(skillsDir, "keep.txt")) + assert.NoError(t, fileErr) +} + +func TestSkillsRemoveFromWorkspaceUsesLastPathSegment(t *testing.T) { + workspace := t.TempDir() + targetDir := filepath.Join(workspace, "skills", "pr-review") + require.NoError(t, os.MkdirAll(targetDir, 0o755)) + + err := skillsRemoveFromWorkspace( + workspace, + config.DefaultConfig().Tools.Skills, + "https://github.com/foo/bar/tree/main/.agents/skills/pr-review", + ) + require.NoError(t, err) + + _, statErr := os.Stat(targetDir) + assert.True(t, os.IsNotExist(statErr)) +} + +func TestSkillsRemoveFromWorkspaceSupportsRepoRootGitHubBlobURL(t *testing.T) { + workspace := t.TempDir() + targetDir := filepath.Join(workspace, "skills", "bar") + require.NoError(t, os.MkdirAll(targetDir, 0o755)) + + err := skillsRemoveFromWorkspace( + workspace, + config.DefaultConfig().Tools.Skills, + "https://github.com/foo/bar/blob/feature/skills-registry/SKILL.md", + ) + require.NoError(t, err) + + _, statErr := os.Stat(targetDir) + assert.True(t, os.IsNotExist(statErr)) +} + +func TestSkillsRemoveFromWorkspaceSupportsGitHubEnterpriseURL(t *testing.T) { + workspace := t.TempDir() + targetDir := filepath.Join(workspace, "skills", "pr-review") + require.NoError(t, os.MkdirAll(targetDir, 0o755)) + + cfg := config.DefaultConfig() + githubRegistry, ok := cfg.Tools.Skills.Registries.Get("github") + require.True(t, ok) + githubRegistry.BaseURL = "https://ghe.example.com/git" + cfg.Tools.Skills.Registries.Set("github", githubRegistry) + + err := skillsRemoveFromWorkspace( + workspace, + cfg.Tools.Skills, + "https://ghe.example.com/git/foo/bar/tree/main/.agents/skills/pr-review", + ) + require.NoError(t, err) + + _, statErr := os.Stat(targetDir) + assert.True(t, os.IsNotExist(statErr)) +} + +func TestSkillsRemoveFromWorkspaceDoesNotRequireEnabledGitHubRegistry(t *testing.T) { + workspace := t.TempDir() + targetDir := filepath.Join(workspace, "skills", "pr-review") + require.NoError(t, os.MkdirAll(targetDir, 0o755)) + + cfg := config.DefaultConfig() + githubRegistry, ok := cfg.Tools.Skills.Registries.Get("github") + require.True(t, ok) + githubRegistry.Enabled = false + cfg.Tools.Skills.Registries.Set("github", githubRegistry) + + err := skillsRemoveFromWorkspace( + workspace, + cfg.Tools.Skills, + "https://github.com/foo/bar/tree/main/.agents/skills/pr-review", + ) + require.NoError(t, err) + + _, statErr := os.Stat(targetDir) + assert.True(t, os.IsNotExist(statErr)) +} diff --git a/cmd/picoclaw/internal/skills/remove.go b/cmd/picoclaw/internal/skills/remove.go index 2b30e94a2..4c9a44d8d 100644 --- a/cmd/picoclaw/internal/skills/remove.go +++ b/cmd/picoclaw/internal/skills/remove.go @@ -2,9 +2,11 @@ package skills import ( "github.com/spf13/cobra" + + "github.com/sipeed/picoclaw/cmd/picoclaw/internal" ) -func newRemoveCommand(workspaceFn func() (string, error)) *cobra.Command { +func newRemoveCommand() *cobra.Command { cmd := &cobra.Command{ Use: "remove", Aliases: []string{"rm", "uninstall"}, @@ -12,11 +14,11 @@ func newRemoveCommand(workspaceFn func() (string, error)) *cobra.Command { Args: cobra.ExactArgs(1), Example: `picoclaw skills remove weather`, RunE: func(_ *cobra.Command, args []string) error { - workspace, err := workspaceFn() + cfg, err := internal.LoadConfig() if err != nil { return err } - return skillsRemoveFromWorkspace(workspace, args[0]) + return skillsRemoveFromWorkspace(cfg.WorkspacePath(), cfg.Tools.Skills, args[0]) }, } diff --git a/cmd/picoclaw/internal/skills/remove_test.go b/cmd/picoclaw/internal/skills/remove_test.go index b4c79760c..cc4d94a09 100644 --- a/cmd/picoclaw/internal/skills/remove_test.go +++ b/cmd/picoclaw/internal/skills/remove_test.go @@ -8,7 +8,7 @@ import ( ) func TestNewRemoveSubcommand(t *testing.T) { - cmd := newRemoveCommand(nil) + cmd := newRemoveCommand() require.NotNil(t, cmd) diff --git a/pkg/agent/hooks_test.go b/pkg/agent/hooks_test.go index 9049a5c72..40ec59c70 100644 --- a/pkg/agent/hooks_test.go +++ b/pkg/agent/hooks_test.go @@ -819,9 +819,26 @@ func TestAgentLoop_HookRespond_SteeringSkipsRemaining(t *testing.T) { resultCh <- result{resp: resp, err: err} }() - time.Sleep(50 * time.Millisecond) - - al.Steer(providers.Message{Role: "user", Content: "change direction"}) + collectedEvents := make([]Event, 0, 8) + steered := false + deadline := time.After(3 * time.Second) + for !steered { + select { + case evt := <-sub.C: + collectedEvents = append(collectedEvents, evt) + if evt.Kind != EventKindToolExecEnd { + continue + } + payload, ok := evt.Payload.(ToolExecEndPayload) + if !ok || payload.Tool != "tool_one" { + continue + } + al.Steer(providers.Message{Role: "user", Content: "change direction"}) + steered = true + case <-deadline: + t.Fatal("timeout waiting for tool_one to finish before steering") + } + } select { case r := <-resultCh: @@ -832,7 +849,7 @@ func TestAgentLoop_HookRespond_SteeringSkipsRemaining(t *testing.T) { t.Fatal("timeout waiting for result") } - events := collectEventStream(sub.C) + events := append(collectedEvents, collectEventStream(sub.C)...) skippedEvts := filterEvents(events, EventKindToolExecSkipped) if len(skippedEvts) < 1 { diff --git a/pkg/config/config.go b/pkg/config/config.go index 218585a79..3e08f076d 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -989,6 +989,10 @@ const ( envSkillsClawHubTimeout = "PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_TIMEOUT" envSkillsClawHubMaxZipSize = "PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_MAX_ZIP_SIZE" envSkillsClawHubMaxResponseSize = "PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_MAX_RESPONSE_SIZE" + envSkillsGitHubEnabled = "PICOCLAW_SKILLS_REGISTRIES_GITHUB_ENABLED" + envSkillsGitHubBaseURL = "PICOCLAW_SKILLS_REGISTRIES_GITHUB_BASE_URL" + envSkillsGitHubAuthToken = "PICOCLAW_SKILLS_REGISTRIES_GITHUB_AUTH_TOKEN" + envSkillsGitHubProxy = "PICOCLAW_SKILLS_REGISTRIES_GITHUB_PROXY" ) func (c *SkillRegistryConfig) DecodeParam(target any) error { @@ -1201,8 +1205,8 @@ func applySkillsRegistryEnvCompat(cfg *Config) { return } - registryCfg, ok := cfg.Tools.Skills.Registries.Get("clawhub") - if !ok { + registryCfg, foundClawHub := cfg.Tools.Skills.Registries.Get("clawhub") + if !foundClawHub { registryCfg = SkillRegistryConfig{ Name: "clawhub", Param: map[string]any{}, @@ -1212,43 +1216,71 @@ func applySkillsRegistryEnvCompat(cfg *Config) { registryCfg.Param = map[string]any{} } - if raw, ok := os.LookupEnv(envSkillsClawHubEnabled); ok { + if raw, envSet := os.LookupEnv(envSkillsClawHubEnabled); envSet { if value, err := strconv.ParseBool(strings.TrimSpace(raw)); err == nil { registryCfg.Enabled = value } } - if value, ok := os.LookupEnv(envSkillsClawHubBaseURL); ok { + if value, envSet := os.LookupEnv(envSkillsClawHubBaseURL); envSet { registryCfg.BaseURL = value } - if value, ok := os.LookupEnv(envSkillsClawHubAuthToken); ok { + if value, envSet := os.LookupEnv(envSkillsClawHubAuthToken); envSet { registryCfg.AuthToken = *NewSecureString(value) } - if value, ok := os.LookupEnv(envSkillsClawHubSearchPath); ok { + if value, envSet := os.LookupEnv(envSkillsClawHubSearchPath); envSet { registryCfg.Param["search_path"] = value } - if value, ok := os.LookupEnv(envSkillsClawHubSkillsPath); ok { + if value, envSet := os.LookupEnv(envSkillsClawHubSkillsPath); envSet { registryCfg.Param["skills_path"] = value } - if value, ok := os.LookupEnv(envSkillsClawHubDownloadPath); ok { + if value, envSet := os.LookupEnv(envSkillsClawHubDownloadPath); envSet { registryCfg.Param["download_path"] = value } - if raw, ok := os.LookupEnv(envSkillsClawHubTimeout); ok { + if raw, envSet := os.LookupEnv(envSkillsClawHubTimeout); envSet { if value, err := strconv.Atoi(strings.TrimSpace(raw)); err == nil { registryCfg.Param["timeout"] = value } } - if raw, ok := os.LookupEnv(envSkillsClawHubMaxZipSize); ok { + if raw, envSet := os.LookupEnv(envSkillsClawHubMaxZipSize); envSet { if value, err := strconv.Atoi(strings.TrimSpace(raw)); err == nil { registryCfg.Param["max_zip_size"] = value } } - if raw, ok := os.LookupEnv(envSkillsClawHubMaxResponseSize); ok { + if raw, envSet := os.LookupEnv(envSkillsClawHubMaxResponseSize); envSet { if value, err := strconv.Atoi(strings.TrimSpace(raw)); err == nil { registryCfg.Param["max_response_size"] = value } } cfg.Tools.Skills.Registries.Set("clawhub", registryCfg) + + githubCfg, foundGitHub := cfg.Tools.Skills.Registries.Get("github") + if !foundGitHub { + githubCfg = SkillRegistryConfig{ + Name: "github", + Param: map[string]any{}, + } + } + if githubCfg.Param == nil { + githubCfg.Param = map[string]any{} + } + + if raw, envSet := os.LookupEnv(envSkillsGitHubEnabled); envSet { + if value, err := strconv.ParseBool(strings.TrimSpace(raw)); err == nil { + githubCfg.Enabled = value + } + } + if value, envSet := os.LookupEnv(envSkillsGitHubBaseURL); envSet { + githubCfg.BaseURL = value + } + if value, envSet := os.LookupEnv(envSkillsGitHubAuthToken); envSet { + githubCfg.AuthToken = *NewSecureString(value) + } + if value, envSet := os.LookupEnv(envSkillsGitHubProxy); envSet { + githubCfg.Param["proxy"] = value + } + + cfg.Tools.Skills.Registries.Set("github", githubCfg) } func makeBackup(path string) error { diff --git a/pkg/config/config_struct.go b/pkg/config/config_struct.go index 17738959a..02f22b5c7 100644 --- a/pkg/config/config_struct.go +++ b/pkg/config/config_struct.go @@ -454,6 +454,10 @@ func (c *SkillRegistryConfig) UnmarshalJSON(data []byte) error { switch key { case "name", "enabled", "base_url", "auth_token", "param": continue + case "_auth_token": + // UI/API shadow secret fields should hydrate SecureString only and must + // never be persisted as arbitrary registry params. + continue default: var decoded any if err := json.Unmarshal(value, &decoded); err != nil { @@ -475,7 +479,7 @@ func (c SkillRegistryConfig) MarshalJSON() ([]byte, error) { m["auth_token"] = c.AuthToken } for key, value := range c.Param { - if key == "" || key == "param" { + if key == "" || key == "param" || strings.HasPrefix(key, "_") { continue } if _, exists := m[key]; exists { @@ -522,6 +526,10 @@ func (c *SkillRegistryConfig) UnmarshalYAML(value *yaml.Node) error { if err := yaml.Unmarshal(data, &c.AuthToken); err != nil { return err } + case "_auth_token": + // UI/API shadow secret fields should hydrate SecureString only and must + // never be persisted as arbitrary registry params. + continue case "param": continue default: @@ -542,7 +550,7 @@ func (c SkillRegistryConfig) MarshalYAML() (any, error) { } keys := make([]string, 0, len(c.Param)) for key := range c.Param { - if key == "" || key == "param" { + if key == "" || key == "param" || strings.HasPrefix(key, "_") { continue } keys = append(keys, key) diff --git a/pkg/config/config_struct_test.go b/pkg/config/config_struct_test.go index 422c9978f..280db5d0f 100644 --- a/pkg/config/config_struct_test.go +++ b/pkg/config/config_struct_test.go @@ -181,6 +181,47 @@ func TestSkillRegistryConfigJSONFlattensParam(t *testing.T) { assert.Equal(t, "http://127.0.0.1:7890", loaded.Param["proxy"]) } +func TestSkillRegistryConfigJSONIgnoresShadowSecretFields(t *testing.T) { + var registry SkillRegistryConfig + err := json.Unmarshal([]byte(`{ + "enabled": true, + "base_url": "https://github.com", + "_auth_token": "shadow-secret", + "proxy": "http://127.0.0.1:7890" + }`), ®istry) + assert.NoError(t, err) + assert.Equal(t, "https://github.com", registry.BaseURL) + assert.Equal(t, "http://127.0.0.1:7890", registry.Param["proxy"]) + _, exists := registry.Param["_auth_token"] + assert.False(t, exists) + + registry.Param["_auth_token"] = "should-not-round-trip" + data, err := json.Marshal(registry) + assert.NoError(t, err) + assert.NotContains(t, string(data), "_auth_token") + assert.Contains(t, string(data), `"proxy":"http://127.0.0.1:7890"`) + + yamlData, err := yaml.Marshal(registry) + assert.NoError(t, err) + assert.NotContains(t, string(yamlData), "_auth_token") + assert.Contains(t, string(yamlData), "proxy: http://127.0.0.1:7890") +} + +func TestSkillRegistryConfigYAMLIgnoresShadowSecretFields(t *testing.T) { + var registry SkillRegistryConfig + err := yaml.Unmarshal([]byte(` +enabled: true +base_url: https://github.com +_auth_token: shadow-secret +proxy: http://127.0.0.1:7890 +`), ®istry) + assert.NoError(t, err) + assert.Equal(t, "https://github.com", registry.BaseURL) + assert.Equal(t, "http://127.0.0.1:7890", registry.Param["proxy"]) + _, exists := registry.Param["_auth_token"] + assert.False(t, exists) +} + func TestSkillsRegistriesConfigMarshalYAMLIncludesRegistryToken(t *testing.T) { registries := SkillsRegistriesConfig{ &SkillRegistryConfig{ diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 5d29591bb..2fa8d9e39 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -1567,6 +1567,42 @@ func TestLoadConfig_AppliesLegacyClawHubRegistryEnvOverrides(t *testing.T) { } } +func TestLoadConfig_AppliesGitHubRegistryEnvOverrides(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.json") + data := `{"version":2,"tools":{"skills":{"registries":{"github":{"enabled":true,"base_url":"https://github.com"}}}}}` + if err := os.WriteFile(cfgPath, []byte(data), 0o600); err != nil { + t.Fatalf("setup: %v", err) + } + + t.Setenv(envSkillsGitHubBaseURL, "https://ghe.example.com/git") + t.Setenv(envSkillsGitHubAuthToken, "github-token-from-env") + t.Setenv(envSkillsGitHubEnabled, "false") + t.Setenv(envSkillsGitHubProxy, "http://127.0.0.1:7890") + + cfg, err := LoadConfig(cfgPath) + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + + github, ok := cfg.Tools.Skills.Registries.Get("github") + if !ok { + t.Fatal("github registry missing") + } + if github.BaseURL != "https://ghe.example.com/git" { + t.Fatalf("BaseURL = %q, want %q", github.BaseURL, "https://ghe.example.com/git") + } + if github.AuthToken.String() != "github-token-from-env" { + t.Fatalf("AuthToken = %q, want %q", github.AuthToken.String(), "github-token-from-env") + } + if github.Enabled { + t.Fatal("Enabled = true, want false") + } + if got := github.Param["proxy"]; got != "http://127.0.0.1:7890" { + t.Fatalf("proxy = %v, want %q", got, "http://127.0.0.1:7890") + } +} + func TestModelConfig_ExtraBodyRoundTrip(t *testing.T) { dir := t.TempDir() cfgPath := filepath.Join(dir, "config.json") diff --git a/pkg/config/security.go b/pkg/config/security.go index 2414cd7fa..094dd5e62 100644 --- a/pkg/config/security.go +++ b/pkg/config/security.go @@ -46,6 +46,102 @@ func loadSecurityConfig(cfg *Config, securityPath string) error { if err := yaml.Unmarshal(data, cfg); err != nil { return fmt.Errorf("failed to parse security config: %w", err) } + if err := applyLegacySkillsSecurityConfig(cfg, data); err != nil { + return fmt.Errorf("failed to parse legacy skills security config: %w", err) + } + + return nil +} + +func applyLegacySkillsSecurityConfig(cfg *Config, data []byte) error { + var root yaml.Node + if err := yaml.Unmarshal(data, &root); err != nil { + return err + } + if len(root.Content) == 0 { + return nil + } + + rootMap := root.Content[0] + if rootMap == nil || rootMap.Kind != yaml.MappingNode { + return nil + } + + for i := 0; i+1 < len(rootMap.Content); i += 2 { + keyNode := rootMap.Content[i] + valueNode := rootMap.Content[i+1] + if keyNode == nil || valueNode == nil || strings.TrimSpace(keyNode.Value) != "skills" { + continue + } + return applyLegacySkillsSecurityNode(cfg, valueNode) + } + + return nil +} + +func applyLegacySkillsSecurityNode(cfg *Config, skillsNode *yaml.Node) error { + if cfg == nil || skillsNode == nil || skillsNode.Kind != yaml.MappingNode { + return nil + } + + for i := 0; i+1 < len(skillsNode.Content); i += 2 { + nameNode := skillsNode.Content[i] + valueNode := skillsNode.Content[i+1] + if nameNode == nil || valueNode == nil { + continue + } + + name := strings.TrimSpace(nameNode.Value) + if name == "" || name == "registries" { + continue + } + + if name == "github" { + var legacyGitHub SkillsGithubConfig + if err := valueNode.Decode(&legacyGitHub); err != nil { + return err + } + if cfg.Tools.Skills.Github.Token.String() == "" && legacyGitHub.Token.String() != "" { + cfg.Tools.Skills.Github.Token = legacyGitHub.Token + } + } + + var legacyRegistry SkillRegistryConfig + if err := valueNode.Decode(&legacyRegistry); err != nil { + return err + } + legacyRegistry.Name = name + if legacyRegistry.AuthToken.String() == "" { + if name == "github" && cfg.Tools.Skills.Github.Token.String() != "" { + legacyRegistry.AuthToken = cfg.Tools.Skills.Github.Token + } else { + continue + } + } + + registryCfg, ok := cfg.Tools.Skills.Registries.Get(name) + if !ok { + registryCfg = SkillRegistryConfig{ + Name: name, + Param: map[string]any{}, + } + } + if registryCfg.Param == nil { + registryCfg.Param = map[string]any{} + } + if registryCfg.AuthToken.String() == "" { + registryCfg.AuthToken = legacyRegistry.AuthToken + } + if registryCfg.BaseURL == "" && legacyRegistry.BaseURL != "" { + registryCfg.BaseURL = legacyRegistry.BaseURL + } + for key, value := range legacyRegistry.Param { + if _, exists := registryCfg.Param[key]; !exists { + registryCfg.Param[key] = value + } + } + cfg.Tools.Skills.Registries.Set(name, registryCfg) + } return nil } diff --git a/pkg/config/security_integration_test.go b/pkg/config/security_integration_test.go index c97955adb..22390acca 100644 --- a/pkg/config/security_integration_test.go +++ b/pkg/config/security_integration_test.go @@ -527,4 +527,77 @@ skills: require.True(t, ok) assert.Equal(t, "https://github.com", githubRegistry.BaseURL) }) + + t.Run("Legacy direct registry security entries remain supported", func(t *testing.T) { + tmpDir := t.TempDir() + + configPath := filepath.Join(tmpDir, "config.json") + configContent := `{ + "version": 1, + "tools": { + "skills": { + "registries": { + "clawhub": { + "enabled": true, + "base_url": "https://clawhub.ai" + } + } + } + } +}` + err := os.WriteFile(configPath, []byte(configContent), 0o644) + require.NoError(t, err) + + securityPath := filepath.Join(tmpDir, SecurityConfigFile) + securityContent := `skills: + clawhub: + auth_token: "legacy-clawhub-token" +` + err = os.WriteFile(securityPath, []byte(securityContent), 0o600) + require.NoError(t, err) + + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + + registry, ok := cfg.Tools.Skills.Registries.Get("clawhub") + require.True(t, ok) + assert.Equal(t, "legacy-clawhub-token", registry.AuthToken.String()) + }) + + t.Run("Legacy github security token populates github registry", func(t *testing.T) { + tmpDir := t.TempDir() + + configPath := filepath.Join(tmpDir, "config.json") + configContent := `{ + "version": 1, + "tools": { + "skills": { + "registries": { + "github": { + "enabled": true, + "base_url": "https://github.com" + } + } + } + } +}` + err := os.WriteFile(configPath, []byte(configContent), 0o644) + require.NoError(t, err) + + securityPath := filepath.Join(tmpDir, SecurityConfigFile) + securityContent := `skills: + github: + token: "legacy-github-token" +` + err = os.WriteFile(securityPath, []byte(securityContent), 0o600) + require.NoError(t, err) + + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + + registry, ok := cfg.Tools.Skills.Registries.Get("github") + require.True(t, ok) + assert.Equal(t, "legacy-github-token", cfg.Tools.Skills.Github.Token.String()) + assert.Equal(t, "legacy-github-token", registry.AuthToken.String()) + }) } diff --git a/pkg/skills/config_bridge.go b/pkg/skills/config_bridge.go index 9f92d6103..5302db196 100644 --- a/pkg/skills/config_bridge.go +++ b/pkg/skills/config_bridge.go @@ -95,6 +95,15 @@ func LookupRegistryFromToolsConfig(cfg config.SkillsToolsConfig, name string) Sk return nil } +func GitHubInstallDirNameFromToolsConfig(cfg config.SkillsToolsConfig, target string) (string, error) { + registryCfg, ok := cfg.Registries.Get("github") + if ok { + registryCfg = applyLegacyGithubRegistryCompatibility(cfg, registryCfg) + return githubInstallDirNameWithBaseURL(target, registryCfg.BaseURL) + } + return githubInstallDirNameWithBaseURL(target, cfg.Github.BaseURL) +} + func NormalizeInstallTargetForRegistry(cfg config.SkillsToolsConfig, registryName, target string) string { if registryName == "" || target == "" { return target diff --git a/pkg/skills/github_registry.go b/pkg/skills/github_registry.go index 79c76f035..de2dd9697 100644 --- a/pkg/skills/github_registry.go +++ b/pkg/skills/github_registry.go @@ -90,16 +90,19 @@ func (r *GitHubRegistry) SkillURL(target, version string) string { ref := parsedTarget.Ref base := strings.TrimRight(parsedTarget.Endpoints.WebBaseURL, "/") urlPath := path.Join(ref.Owner, ref.RepoName) - if ref.Ref == "" { - return fmt.Sprintf("%s/%s", base, urlPath) - } if ref.SubPath != "" { + if ref.Ref == "" { + return "" + } viewKind := "tree" if isSkillMarkdownPath(ref.SubPath) { viewKind = "blob" } return fmt.Sprintf("%s/%s/%s/%s/%s", base, urlPath, viewKind, ref.Ref, ref.SubPath) } + if ref.Ref == "" { + return fmt.Sprintf("%s/%s", base, urlPath) + } if ref.Ref != "main" { return fmt.Sprintf("%s/%s/tree/%s", base, urlPath, ref.Ref) } diff --git a/pkg/skills/github_registry_test.go b/pkg/skills/github_registry_test.go index 69f5adc91..3ac309700 100644 --- a/pkg/skills/github_registry_test.go +++ b/pkg/skills/github_registry_test.go @@ -186,6 +186,7 @@ func TestGitHubRegistrySkillURLUsesProvidedVersionAndBasePath(t *testing.T) { "https://github.com/org/repo/tree/main/.agents/skills/pr-review", registry.SkillURL("https://github.com/org/repo/tree/main/.agents/skills/pr-review", ""), ) + assert.Empty(t, registry.SkillURL("org/repo/.agents/skills/pr-review", "")) } func TestGitHubRegistryResolveInstallDirNameSupportsFullURLs(t *testing.T) { diff --git a/pkg/skills/installer.go b/pkg/skills/installer.go index 43985f2c2..2f97ca8bf 100644 --- a/pkg/skills/installer.go +++ b/pkg/skills/installer.go @@ -210,32 +210,23 @@ func splitGitHubTreeOrBlobRefPath(parts []string, defaultRef string) (string, st if len(parts) == 0 { return defaultRef, "" } - for i := 1; i < len(parts); i++ { - candidateRef := strings.Join(parts[:i], "/") - candidateSubPath := strings.Join(parts[i:], "/") - if looksLikeSkillSubPath(candidateSubPath) { - return candidateRef, candidateSubPath - } + if anchor := knownSkillSubPathAnchor(parts); anchor > 0 { + return strings.Join(parts[:anchor], "/"), strings.Join(parts[anchor:], "/") + } + if parts[len(parts)-1] == "SKILL.md" { + return strings.Join(parts[:len(parts)-1], "/"), "SKILL.md" } return parts[0], strings.Join(parts[1:], "/") } -func looksLikeSkillSubPath(subPath string) bool { - subPath = strings.Trim(strings.TrimSpace(subPath), "/") - if subPath == "" { - return false +func knownSkillSubPathAnchor(parts []string) int { + for i := 1; i < len(parts); i++ { + candidateSubPath := strings.Join(parts[i:], "/") + if strings.HasPrefix(candidateSubPath, ".agents/skills/") || strings.HasPrefix(candidateSubPath, "skills/") { + return i + } } - if isSkillMarkdownPath(subPath) { - return true - } - parts := strings.Split(subPath, "/") - if len(parts) >= 2 && parts[0] == "skills" { - return true - } - if len(parts) >= 3 && parts[0] == ".agents" && parts[1] == "skills" { - return true - } - return false + return -1 } func isSkillMarkdownPath(subPath string) bool { @@ -437,11 +428,19 @@ func (si *SkillInstaller) InstallFromGitHubToDir( return nil, err } ref := target.Ref + apiSubPath := strings.Trim(ref.SubPath, "/") + if isSkillMarkdownPath(apiSubPath) { + if dir := path.Dir(apiSubPath); dir == "." { + apiSubPath = "" + } else { + apiSubPath = dir + } + } // Build GitHub API URL apiPath := path.Join(ref.Owner, ref.RepoName, "contents") - if ref.SubPath != "" { - apiPath = path.Join(apiPath, ref.SubPath) + if apiSubPath != "" { + apiPath = path.Join(apiPath, apiSubPath) } apiURL := fmt.Sprintf("%s/repos/%s?ref=%s", target.Endpoints.APIBaseURL, apiPath, url.QueryEscape(ref.Ref)) diff --git a/pkg/skills/installer_test.go b/pkg/skills/installer_test.go index 683689b35..9691a5312 100644 --- a/pkg/skills/installer_test.go +++ b/pkg/skills/installer_test.go @@ -289,6 +289,23 @@ func TestParseGitHubTargetWithBaseURLPreservesSourceEndpoints(t *testing.T) { } } +func TestParseGitHubTargetWithBaseURLPreservesSlashBranchForRepoRootBlobSkill(t *testing.T) { + target, err := parseGitHubTargetWithBaseURL( + "https://github.com/org/repo/blob/feature/skills-registry/SKILL.md", + "", + "", + ) + if err != nil { + t.Fatalf("parseGitHubTargetWithBaseURL() unexpected error = %v", err) + } + if target.Ref.Ref != "feature/skills-registry" { + t.Fatalf("ref = %q, want feature/skills-registry", target.Ref.Ref) + } + if target.Ref.SubPath != "SKILL.md" { + t.Fatalf("subPath = %q, want SKILL.md", target.Ref.SubPath) + } +} + func TestSkillInstallerResolveGitHubRefUsesDefaultBranch(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { @@ -324,11 +341,21 @@ func TestSkillInstallerInstallFromGitHubToDirSupportsBlobSkillURL(t *testing.T) var server *httptest.Server server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { - case "/api/v3/repos/org/repo/contents/.agents/skills/pr-review/SKILL.md": + case "/api/v3/repos/org/repo/contents/.agents/skills/pr-review": w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"type":"file","name":"SKILL.md"}`)) + _, _ = w.Write([]byte(`[ + {"type":"file","name":"SKILL.md","download_url":"` + server.URL + `/raw/org/repo/main/.agents/skills/pr-review/SKILL.md"}, + {"type":"dir","name":"scripts","url":"` + server.URL + `/api/v3/repos/org/repo/contents/.agents/skills/pr-review/scripts?ref=main"} + ]`)) + case "/api/v3/repos/org/repo/contents/.agents/skills/pr-review/scripts": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[ + {"type":"file","name":"check.sh","download_url":"` + server.URL + `/raw/org/repo/main/.agents/skills/pr-review/scripts/check.sh"} + ]`)) case "/raw/org/repo/main/.agents/skills/pr-review/SKILL.md": _, _ = w.Write([]byte("---\nname: pr-review\ndescription: PR review skill\n---\n# PR Review\n")) + case "/raw/org/repo/main/.agents/skills/pr-review/scripts/check.sh": + _, _ = w.Write([]byte("#!/bin/sh\nexit 0\n")) default: t.Fatalf("unexpected path: %s", r.URL.Path) } @@ -361,6 +388,11 @@ func TestSkillInstallerInstallFromGitHubToDirSupportsBlobSkillURL(t *testing.T) if !strings.Contains(string(content), "name: pr-review") { t.Fatalf("SKILL.md content = %q, want skill metadata", string(content)) } + + scriptPath := filepath.Join(targetDir, "scripts", "check.sh") + if _, err := os.Stat(scriptPath); err != nil { + t.Fatalf("Stat(scripts/check.sh) error = %v", err) + } } func TestShouldDownload(t *testing.T) { diff --git a/web/backend/api/config_test.go b/web/backend/api/config_test.go index a90145f3c..034ef68e4 100644 --- a/web/backend/api/config_test.go +++ b/web/backend/api/config_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "github.com/sipeed/picoclaw/pkg/config" @@ -379,6 +380,57 @@ func TestHandlePatchConfig_SavesDiscordTokenFromPayload(t *testing.T) { } } +func TestHandlePatchConfig_DoesNotPersistShadowRegistryAuthTokenField(t *testing.T) { + configPath, cleanup := setupOAuthTestEnv(t) + defer cleanup() + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + req := httptest.NewRequest(http.MethodPatch, "/api/config", bytes.NewBufferString(`{ + "tools": { + "skills": { + "registries": { + "github": { + "_auth_token": "ghp-shadow-token" + } + } + } + } + }`)) + req.Header.Set("Content-Type", "application/json") + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("PATCH /api/config status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } + + cfg, err := config.LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + githubRegistry, ok := cfg.Tools.Skills.Registries.Get("github") + if !ok { + t.Fatal("github registry missing after PATCH") + } + if got := githubRegistry.AuthToken.String(); got != "ghp-shadow-token" { + t.Fatalf("github registry auth token = %q, want %q", got, "ghp-shadow-token") + } + if got := githubRegistry.BaseURL; got != "https://github.com" { + t.Fatalf("github registry base_url = %q, want %q", got, "https://github.com") + } + + rawConfig, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("ReadFile(configPath) error = %v", err) + } + if strings.Contains(string(rawConfig), "_auth_token") { + t.Fatalf("config.json should not persist _auth_token shadow field, got:\n%s", string(rawConfig)) + } +} + func TestHandlePatchConfig_AllowsInvalidDenyRegexPatternsWhenDenyPatternsDisabled(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup() diff --git a/web/backend/api/pico_test.go b/web/backend/api/pico_test.go index af5ba205f..d532d89c9 100644 --- a/web/backend/api/pico_test.go +++ b/web/backend/api/pico_test.go @@ -567,7 +567,11 @@ func TestHandleWebSocketProxyLoadsPidDataOnDemand(t *testing.T) { } func TestHandleWebSocketProxyRejectsStalePidDataAfterProcessExit(t *testing.T) { - configPath := filepath.Join(t.TempDir(), "config.json") + tmpDir := t.TempDir() + t.Setenv("HOME", tmpDir) + t.Setenv("PICOCLAW_HOME", filepath.Join(tmpDir, ".picoclaw")) + + configPath := filepath.Join(tmpDir, "config.json") h := NewHandler(configPath) handler := h.handleWebSocketProxy() diff --git a/web/backend/api/skills.go b/web/backend/api/skills.go index 8d8995fd9..e89ff7c30 100644 --- a/web/backend/api/skills.go +++ b/web/backend/api/skills.go @@ -493,13 +493,14 @@ func (h *Handler) handleDeleteSkill(w http.ResponseWriter, r *http.Request) { workspaceSkillWriteMu.Lock() defer workspaceSkillWriteMu.Unlock() + var matchedNonWorkspace bool for _, skill := range loader.ListSkills() { if skill.Name != name { continue } if skill.Source != "workspace" { - http.Error(w, "only workspace skills can be deleted", http.StatusBadRequest) - return + matchedNonWorkspace = true + continue } if err := os.RemoveAll(filepath.Dir(skill.Path)); err != nil { http.Error(w, fmt.Sprintf("Failed to delete skill: %v", err), http.StatusInternalServerError) @@ -509,6 +510,10 @@ func (h *Handler) handleDeleteSkill(w http.ResponseWriter, r *http.Request) { json.NewEncoder(w).Encode(map[string]string{"status": "ok"}) return } + if matchedNonWorkspace { + http.Error(w, "only workspace skills can be deleted", http.StatusBadRequest) + return + } http.Error(w, "Skill not found", http.StatusNotFound) } diff --git a/web/backend/api/skills_test.go b/web/backend/api/skills_test.go index 32dd5f7f3..977ec693f 100644 --- a/web/backend/api/skills_test.go +++ b/web/backend/api/skills_test.go @@ -549,6 +549,65 @@ func TestHandleDeleteSkill(t *testing.T) { } } +func TestHandleDeleteSkillPrefersWorkspaceMatch(t *testing.T) { + configPath, cleanup := setupOAuthTestEnv(t) + defer cleanup() + + cfg, err := config.LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + homeDir := t.TempDir() + t.Setenv(config.EnvHome, homeDir) + workspace := filepath.Join(t.TempDir(), "workspace") + cfg.Agents.Defaults.Workspace = workspace + if err := config.SaveConfig(configPath, cfg); err != nil { + t.Fatalf("SaveConfig() error = %v", err) + } + + workspaceSkillDir := filepath.Join(workspace, "skills", "delete-me-workspace") + if err := os.MkdirAll(workspaceSkillDir, 0o755); err != nil { + t.Fatalf("MkdirAll(workspace) error = %v", err) + } + if err := os.WriteFile( + filepath.Join(workspaceSkillDir, "SKILL.md"), + []byte("---\nname: delete-me\ndescription: workspace delete me\n---\n"), + 0o644, + ); err != nil { + t.Fatalf("WriteFile(workspace) error = %v", err) + } + + globalSkillDir := filepath.Join(homeDir, "skills", "delete-me-global") + if err := os.MkdirAll(globalSkillDir, 0o755); err != nil { + t.Fatalf("MkdirAll(global) error = %v", err) + } + if err := os.WriteFile( + filepath.Join(globalSkillDir, "SKILL.md"), + []byte("---\nname: delete-me\ndescription: global delete me\n---\n"), + 0o644, + ); err != nil { + t.Fatalf("WriteFile(global) error = %v", err) + } + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodDelete, "/api/skills/delete-me", nil) + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } + if _, err := os.Stat(workspaceSkillDir); !os.IsNotExist(err) { + t.Fatalf("workspace skill directory should be removed, stat err=%v", err) + } + if _, err := os.Stat(globalSkillDir); err != nil { + t.Fatalf("global skill directory should remain, stat err=%v", err) + } +} + func TestHandleSearchSkills(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup()