From 1e7902cfac528cbec1405d9b53f0d60aecffb50b Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 08:29:38 +0100 Subject: [PATCH 01/12] Add install state, manifest source interface, and rename skills directory Add state types (InstallState) with atomic load/save persistence, path resolution (GlobalSkillsDir, ProjectSkillsDir stub), ManifestSource interface with GitHub implementation, latest release discovery (FetchLatestRelease), and Clock abstraction. Extend SkillMeta with v2 fields (Experimental, Description, MinCLIVer). Refactor FetchManifest to delegate to GitHubManifestSource. Rename canonical skills directory from .databricks/agent-skills to .databricks/aitools/skills with backward-compat check for the old path. Co-authored-by: Isaac --- experimental/aitools/lib/agents/skills.go | 12 +- .../aitools/lib/agents/skills_test.go | 22 ++++ .../aitools/lib/installer/installer.go | 39 ++---- experimental/aitools/lib/installer/source.go | 112 ++++++++++++++++++ experimental/aitools/lib/installer/state.go | 100 ++++++++++++++++ .../aitools/lib/installer/state_test.go | 78 ++++++++++++ 6 files changed, 329 insertions(+), 34 deletions(-) create mode 100644 experimental/aitools/lib/installer/source.go create mode 100644 experimental/aitools/lib/installer/state.go create mode 100644 experimental/aitools/lib/installer/state_test.go diff --git a/experimental/aitools/lib/agents/skills.go b/experimental/aitools/lib/agents/skills.go index 3c0d67dfbb..0ba522e356 100644 --- a/experimental/aitools/lib/agents/skills.go +++ b/experimental/aitools/lib/agents/skills.go @@ -14,12 +14,15 @@ const ( databricksSkillPrefix = "databricks" // CanonicalSkillsDir is the shared location for skills when multiple agents are detected. - CanonicalSkillsDir = ".databricks/agent-skills" + CanonicalSkillsDir = ".databricks/aitools/skills" + + // legacySkillsDir is the old canonical location, checked for backward compatibility. + legacySkillsDir = ".databricks/agent-skills" ) // HasDatabricksSkillsInstalled checks if Databricks skills are installed in the canonical location. -// Returns true if no agents are detected (nothing to recommend) or if skills exist in ~/.databricks/agent-skills/. -// Only the canonical location is checked so that skills installed by other tools are not mistaken for a proper installation. +// Returns true if no agents are detected (nothing to recommend) or if skills exist in +// ~/.databricks/aitools/skills/ or the legacy ~/.databricks/agent-skills/. func HasDatabricksSkillsInstalled(ctx context.Context) bool { installed := DetectInstalled(ctx) if len(installed) == 0 { @@ -30,7 +33,8 @@ func HasDatabricksSkillsInstalled(ctx context.Context) bool { if err != nil { return false } - return hasDatabricksSkillsIn(filepath.Join(homeDir, CanonicalSkillsDir)) + return hasDatabricksSkillsIn(filepath.Join(homeDir, CanonicalSkillsDir)) || + hasDatabricksSkillsIn(filepath.Join(homeDir, legacySkillsDir)) } // hasDatabricksSkillsIn checks if dir contains a subdirectory starting with "databricks". diff --git a/experimental/aitools/lib/agents/skills_test.go b/experimental/aitools/lib/agents/skills_test.go index 15baba9c8f..09192d721c 100644 --- a/experimental/aitools/lib/agents/skills_test.go +++ b/experimental/aitools/lib/agents/skills_test.go @@ -134,3 +134,25 @@ func TestHasDatabricksSkillsInstalledDatabricksAppsCanonical(t *testing.T) { assert.True(t, HasDatabricksSkillsInstalled(t.Context())) } + +func TestHasDatabricksSkillsInstalledLegacyPath(t *testing.T) { + tmpHome := t.TempDir() + t.Setenv("HOME", tmpHome) + // Skills only in the legacy location should still be detected. + require.NoError(t, os.MkdirAll(filepath.Join(tmpHome, legacySkillsDir, "databricks"), 0o755)) + + agentDir := filepath.Join(tmpHome, ".claude") + require.NoError(t, os.MkdirAll(agentDir, 0o755)) + + origRegistry := Registry + Registry = []Agent{ + { + Name: "test-agent", + DisplayName: "Test Agent", + ConfigDir: func(_ context.Context) (string, error) { return agentDir, nil }, + }, + } + defer func() { Registry = origRegistry }() + + assert.True(t, HasDatabricksSkillsInstalled(t.Context())) +} diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 084e2dc973..b9f2d7b899 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -2,7 +2,6 @@ package installer import ( "context" - "encoding/json" "fmt" "io" "net/http" @@ -13,7 +12,6 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" - "github.com/databricks/cli/libs/log" "github.com/fatih/color" ) @@ -40,39 +38,20 @@ type Manifest struct { // SkillMeta describes a single skill entry in the manifest. type SkillMeta struct { - Version string `json:"version"` - UpdatedAt string `json:"updated_at"` - Files []string `json:"files"` + Version string `json:"version"` + UpdatedAt string `json:"updated_at"` + Files []string `json:"files"` + Experimental bool `json:"experimental,omitempty"` + Description string `json:"description,omitempty"` + MinCLIVer string `json:"min_cli_version,omitempty"` } // FetchManifest fetches the skills manifest from the skills repo. +// This is a convenience wrapper that uses the default GitHubManifestSource. func FetchManifest(ctx context.Context) (*Manifest, error) { + src := &GitHubManifestSource{} ref := getSkillsRef(ctx) - log.Infof(ctx, "Fetching skills manifest from %s/%s@%s", skillsRepoOwner, skillsRepoName, ref) - url := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/manifest.json", - skillsRepoOwner, skillsRepoName, ref) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) - if err != nil { - return nil, fmt.Errorf("failed to create request: %w", err) - } - - client := &http.Client{Timeout: 30 * time.Second} - resp, err := client.Do(req) - if err != nil { - return nil, fmt.Errorf("failed to fetch manifest: %w", err) - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed to fetch manifest: HTTP %d", resp.StatusCode) - } - - var manifest Manifest - if err := json.NewDecoder(resp.Body).Decode(&manifest); err != nil { - return nil, fmt.Errorf("failed to parse manifest: %w", err) - } - - return &manifest, nil + return src.FetchManifest(ctx, ref) } func fetchSkillFile(ctx context.Context, skillName, filePath string) ([]byte, error) { diff --git a/experimental/aitools/lib/installer/source.go b/experimental/aitools/lib/installer/source.go new file mode 100644 index 0000000000..553f04f354 --- /dev/null +++ b/experimental/aitools/lib/installer/source.go @@ -0,0 +1,112 @@ +package installer + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "time" + + "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/log" +) + +// Clock abstracts time for testing. +type Clock interface { + Now() time.Time +} + +// RealClock returns the actual current time. +type RealClock struct{} + +// Now returns the current time. +func (RealClock) Now() time.Time { return time.Now() } + +// ManifestSource abstracts how the skills manifest and release info are fetched. +type ManifestSource interface { + // FetchManifest fetches the skills manifest at the given ref. + FetchManifest(ctx context.Context, ref string) (*Manifest, error) + + // FetchLatestRelease returns the latest release tag. + // Falls back to defaultSkillsRepoRef on any error. + FetchLatestRelease(ctx context.Context) (string, error) +} + +// GitHubManifestSource fetches manifests and release info from GitHub. +type GitHubManifestSource struct{} + +// FetchManifest fetches the skills manifest from GitHub at the given ref. +func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (*Manifest, error) { + log.Infof(ctx, "Fetching skills manifest from %s/%s@%s", skillsRepoOwner, skillsRepoName, ref) + url := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/manifest.json", + skillsRepoOwner, skillsRepoName, ref) + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + + client := &http.Client{Timeout: 30 * time.Second} + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("failed to fetch manifest: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("failed to fetch manifest: HTTP %d", resp.StatusCode) + } + + var manifest Manifest + if err := json.NewDecoder(resp.Body).Decode(&manifest); err != nil { + return nil, fmt.Errorf("failed to parse manifest: %w", err) + } + + return &manifest, nil +} + +// FetchLatestRelease returns the latest release tag from GitHub. +// If DATABRICKS_SKILLS_REF is set, it is returned immediately. +// On any error (network, non-200, parse), falls back to defaultSkillsRepoRef. +func (s *GitHubManifestSource) FetchLatestRelease(ctx context.Context) (string, error) { + if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" { + return ref, nil + } + + url := fmt.Sprintf("https://api.github.com/repos/%s/%s/releases/latest", + skillsRepoOwner, skillsRepoName) + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + log.Debugf(ctx, "Failed to create release request, falling back to %s: %v", defaultSkillsRepoRef, err) + return defaultSkillsRepoRef, nil + } + + client := &http.Client{Timeout: 15 * time.Second} + resp, err := client.Do(req) + if err != nil { + log.Debugf(ctx, "Failed to fetch latest release, falling back to %s: %v", defaultSkillsRepoRef, err) + return defaultSkillsRepoRef, nil + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + log.Debugf(ctx, "Latest release returned HTTP %d, falling back to %s", resp.StatusCode, defaultSkillsRepoRef) + return defaultSkillsRepoRef, nil + } + + var release struct { + TagName string `json:"tag_name"` + } + if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { + log.Debugf(ctx, "Failed to parse release response, falling back to %s: %v", defaultSkillsRepoRef, err) + return defaultSkillsRepoRef, nil + } + + if release.TagName == "" { + log.Debugf(ctx, "Empty tag_name in release response, falling back to %s", defaultSkillsRepoRef) + return defaultSkillsRepoRef, nil + } + + return release.TagName, nil +} diff --git a/experimental/aitools/lib/installer/state.go b/experimental/aitools/lib/installer/state.go new file mode 100644 index 0000000000..deb58ef6cf --- /dev/null +++ b/experimental/aitools/lib/installer/state.go @@ -0,0 +1,100 @@ +package installer + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + + "github.com/databricks/cli/libs/env" +) + +const stateFileName = ".state.json" + +// ErrNotImplemented indicates that a feature is not yet implemented. +var ErrNotImplemented = errors.New("project scope not yet implemented") + +// InstalledSkill records the installed version and timestamp for a single skill. +type InstalledSkill struct { + Version string `json:"version"` + InstalledAt string `json:"installed_at"` +} + +// InstallState records the state of all installed skills in a scope directory. +type InstallState struct { + SchemaVersion int `json:"schema_version"` + SkillsRef string `json:"skills_ref"` + LastChecked string `json:"last_checked,omitempty"` + Skills map[string]InstalledSkill `json:"skills"` +} + +// LoadState reads install state from the given directory. +// Returns (nil, nil) when the state file does not exist. +func LoadState(dir string) (*InstallState, error) { + data, err := os.ReadFile(filepath.Join(dir, stateFileName)) + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("failed to read state file: %w", err) + } + + var state InstallState + if err := json.Unmarshal(data, &state); err != nil { + return nil, fmt.Errorf("failed to parse state file: %w", err) + } + return &state, nil +} + +// SaveState writes install state to the given directory atomically. +// Creates the directory if it does not exist. +func SaveState(dir string, state *InstallState) error { + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("failed to create state directory: %w", err) + } + + data, err := json.MarshalIndent(state, "", " ") + if err != nil { + return fmt.Errorf("failed to marshal state: %w", err) + } + + // Atomic write: write to temp file in the same directory, then rename. + tmp, err := os.CreateTemp(dir, ".state-*.tmp") + if err != nil { + return fmt.Errorf("failed to create temp file: %w", err) + } + tmpName := tmp.Name() + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + os.Remove(tmpName) + return fmt.Errorf("failed to write temp file: %w", err) + } + if err := tmp.Close(); err != nil { + os.Remove(tmpName) + return fmt.Errorf("failed to close temp file: %w", err) + } + + if err := os.Rename(tmpName, filepath.Join(dir, stateFileName)); err != nil { + os.Remove(tmpName) + return fmt.Errorf("failed to rename state file: %w", err) + } + return nil +} + +// GlobalSkillsDir returns the path to the global skills directory (~/.databricks/aitools/skills/). +func GlobalSkillsDir(ctx context.Context) (string, error) { + home, err := env.UserHomeDir(ctx) + if err != nil { + return "", err + } + return filepath.Join(home, ".databricks", "aitools", "skills"), nil +} + +// ProjectSkillsDir returns the path to the project-scoped skills directory. +// Project scope is not yet implemented. +func ProjectSkillsDir(_ context.Context) (string, error) { + return "", ErrNotImplemented +} diff --git a/experimental/aitools/lib/installer/state_test.go b/experimental/aitools/lib/installer/state_test.go new file mode 100644 index 0000000000..0d45dff338 --- /dev/null +++ b/experimental/aitools/lib/installer/state_test.go @@ -0,0 +1,78 @@ +package installer + +import ( + "os" + "path/filepath" + "testing" + + "github.com/databricks/cli/libs/env" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLoadStateNonexistentFile(t *testing.T) { + state, err := LoadState(t.TempDir()) + assert.NoError(t, err) + assert.Nil(t, state) +} + +func TestSaveAndLoadStateRoundtrip(t *testing.T) { + dir := t.TempDir() + original := &InstallState{ + SchemaVersion: 1, + SkillsRef: "v0.2.0", + LastChecked: "2026-03-22T10:00:00Z", + Skills: map[string]InstalledSkill{ + "databricks": { + Version: "1.0.0", + InstalledAt: "2026-03-22T09:00:00Z", + }, + }, + } + + err := SaveState(dir, original) + require.NoError(t, err) + + loaded, err := LoadState(dir) + require.NoError(t, err) + assert.Equal(t, original, loaded) +} + +func TestSaveStateCreatesDirectory(t *testing.T) { + dir := filepath.Join(t.TempDir(), "nested", "path") + state := &InstallState{ + SchemaVersion: 1, + SkillsRef: "v0.1.0", + Skills: map[string]InstalledSkill{}, + } + + err := SaveState(dir, state) + require.NoError(t, err) + + // Verify file exists. + _, err = os.Stat(filepath.Join(dir, stateFileName)) + assert.NoError(t, err) +} + +func TestLoadStateCorruptJSON(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, stateFileName), []byte("{bad json"), 0o644)) + + state, err := LoadState(dir) + assert.Error(t, err) + assert.Nil(t, state) + assert.Contains(t, err.Error(), "failed to parse state file") +} + +func TestGlobalSkillsDir(t *testing.T) { + ctx := env.WithUserHomeDir(t.Context(), "/fake/home") + dir, err := GlobalSkillsDir(ctx) + require.NoError(t, err) + assert.Equal(t, filepath.Join("/fake/home", ".databricks", "aitools", "skills"), dir) +} + +func TestProjectSkillsDirNotImplemented(t *testing.T) { + dir, err := ProjectSkillsDir(t.Context()) + assert.ErrorIs(t, err, ErrNotImplemented) + assert.Empty(t, dir) +} From 418a0aee82d5e77da77d3bd67bdf77fe6be0dbd2 Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 08:36:22 +0100 Subject: [PATCH 02/12] Align InstallState with spec, clean up source.go - Rename SkillsRef->Release, LastChecked->LastUpdated (time.Time), simplify Skills to map[string]string, add IncludeExperimental and Scope fields. - Remove unused Clock/RealClock from source.go. - Add trailing newline to SaveState JSON output. - Improve FetchLatestRelease doc comment on error/fallback contract. - Document intentional DATABRICKS_SKILLS_REF duplication. Co-authored-by: Isaac --- experimental/aitools/lib/installer/source.go | 20 +++---- experimental/aitools/lib/installer/state.go | 18 +++--- .../aitools/lib/installer/state_test.go | 56 ++++++++++++++++--- 3 files changed, 63 insertions(+), 31 deletions(-) diff --git a/experimental/aitools/lib/installer/source.go b/experimental/aitools/lib/installer/source.go index 553f04f354..c32eb721d8 100644 --- a/experimental/aitools/lib/installer/source.go +++ b/experimental/aitools/lib/installer/source.go @@ -11,24 +11,16 @@ import ( "github.com/databricks/cli/libs/log" ) -// Clock abstracts time for testing. -type Clock interface { - Now() time.Time -} - -// RealClock returns the actual current time. -type RealClock struct{} - -// Now returns the current time. -func (RealClock) Now() time.Time { return time.Now() } - // ManifestSource abstracts how the skills manifest and release info are fetched. type ManifestSource interface { // FetchManifest fetches the skills manifest at the given ref. FetchManifest(ctx context.Context, ref string) (*Manifest, error) // FetchLatestRelease returns the latest release tag. - // Falls back to defaultSkillsRepoRef on any error. + // Implementations should fall back to a default ref on network errors rather + // than returning an error. The error return exists for cases where fallback is + // not possible (e.g., mock implementations in tests that want to simulate hard + // failures). FetchLatestRelease(ctx context.Context) (string, error) } @@ -68,6 +60,10 @@ func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (* // FetchLatestRelease returns the latest release tag from GitHub. // If DATABRICKS_SKILLS_REF is set, it is returned immediately. // On any error (network, non-200, parse), falls back to defaultSkillsRepoRef. +// +// The DATABRICKS_SKILLS_REF check is intentionally duplicated in getSkillsRef() +// because callers may use either the ManifestSource interface directly or the +// convenience FetchManifest wrapper. func (s *GitHubManifestSource) FetchLatestRelease(ctx context.Context) (string, error) { if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" { return ref, nil diff --git a/experimental/aitools/lib/installer/state.go b/experimental/aitools/lib/installer/state.go index deb58ef6cf..ec17db1f08 100644 --- a/experimental/aitools/lib/installer/state.go +++ b/experimental/aitools/lib/installer/state.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "time" "github.com/databricks/cli/libs/env" ) @@ -16,18 +17,14 @@ const stateFileName = ".state.json" // ErrNotImplemented indicates that a feature is not yet implemented. var ErrNotImplemented = errors.New("project scope not yet implemented") -// InstalledSkill records the installed version and timestamp for a single skill. -type InstalledSkill struct { - Version string `json:"version"` - InstalledAt string `json:"installed_at"` -} - // InstallState records the state of all installed skills in a scope directory. type InstallState struct { - SchemaVersion int `json:"schema_version"` - SkillsRef string `json:"skills_ref"` - LastChecked string `json:"last_checked,omitempty"` - Skills map[string]InstalledSkill `json:"skills"` + SchemaVersion int `json:"schema_version"` + IncludeExperimental bool `json:"include_experimental,omitempty"` + Release string `json:"release"` + LastUpdated time.Time `json:"last_updated"` + Skills map[string]string `json:"skills"` + Scope string `json:"scope,omitempty"` } // LoadState reads install state from the given directory. @@ -59,6 +56,7 @@ func SaveState(dir string, state *InstallState) error { if err != nil { return fmt.Errorf("failed to marshal state: %w", err) } + data = append(data, '\n') // Atomic write: write to temp file in the same directory, then rename. tmp, err := os.CreateTemp(dir, ".state-*.tmp") diff --git a/experimental/aitools/lib/installer/state_test.go b/experimental/aitools/lib/installer/state_test.go index 0d45dff338..ea459dfafe 100644 --- a/experimental/aitools/lib/installer/state_test.go +++ b/experimental/aitools/lib/installer/state_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/databricks/cli/libs/env" "github.com/stretchr/testify/assert" @@ -20,13 +21,10 @@ func TestSaveAndLoadStateRoundtrip(t *testing.T) { dir := t.TempDir() original := &InstallState{ SchemaVersion: 1, - SkillsRef: "v0.2.0", - LastChecked: "2026-03-22T10:00:00Z", - Skills: map[string]InstalledSkill{ - "databricks": { - Version: "1.0.0", - InstalledAt: "2026-03-22T09:00:00Z", - }, + Release: "v0.2.0", + LastUpdated: time.Date(2026, 3, 22, 10, 0, 0, 0, time.UTC), + Skills: map[string]string{ + "databricks": "1.0.0", }, } @@ -42,8 +40,9 @@ func TestSaveStateCreatesDirectory(t *testing.T) { dir := filepath.Join(t.TempDir(), "nested", "path") state := &InstallState{ SchemaVersion: 1, - SkillsRef: "v0.1.0", - Skills: map[string]InstalledSkill{}, + Release: "v0.1.0", + LastUpdated: time.Date(2026, 3, 22, 9, 0, 0, 0, time.UTC), + Skills: map[string]string{}, } err := SaveState(dir, state) @@ -54,6 +53,23 @@ func TestSaveStateCreatesDirectory(t *testing.T) { assert.NoError(t, err) } +func TestSaveStateTrailingNewline(t *testing.T) { + dir := t.TempDir() + state := &InstallState{ + SchemaVersion: 1, + Release: "v0.1.0", + LastUpdated: time.Date(2026, 3, 22, 9, 0, 0, 0, time.UTC), + Skills: map[string]string{}, + } + + err := SaveState(dir, state) + require.NoError(t, err) + + data, err := os.ReadFile(filepath.Join(dir, stateFileName)) + require.NoError(t, err) + assert.Equal(t, byte('\n'), data[len(data)-1]) +} + func TestLoadStateCorruptJSON(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, stateFileName), []byte("{bad json"), 0o644)) @@ -76,3 +92,25 @@ func TestProjectSkillsDirNotImplemented(t *testing.T) { assert.ErrorIs(t, err, ErrNotImplemented) assert.Empty(t, dir) } + +func TestSaveAndLoadStateWithOptionalFields(t *testing.T) { + dir := t.TempDir() + original := &InstallState{ + SchemaVersion: 1, + IncludeExperimental: true, + Release: "v0.3.0", + LastUpdated: time.Date(2026, 3, 22, 12, 30, 0, 0, time.UTC), + Skills: map[string]string{ + "databricks": "1.0.0", + "sql-tools": "0.2.0", + }, + Scope: "project", + } + + err := SaveState(dir, original) + require.NoError(t, err) + + loaded, err := LoadState(dir) + require.NoError(t, err) + assert.Equal(t, original, loaded) +} From 31756fa2904c30a004c6ee40db7b54fe6c48f9a6 Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 08:57:13 +0100 Subject: [PATCH 03/12] Add InstallSkillsForAgents, interactive agent selection, idempotent install Introduces the core InstallSkillsForAgents function that handles: - Fetching manifest via ManifestSource interface - Filtering experimental skills and enforcing min_cli_version - Idempotent install (skips already-installed skills at same version) - Legacy install detection (skills on disk without state file) - State persistence after successful install - Concise output (two lines: installing header + summary) Adds interactive agent selection in cmd/skills.go using huh multi-select when multiple agents are detected in an interactive terminal. Preserves InstallAllSkills signature (func(context.Context) error) for the cmd/apps/init.go callback. Co-authored-by: Isaac --- experimental/aitools/cmd/install_test.go | 182 +++++++-- experimental/aitools/cmd/skills.go | 74 +++- .../aitools/lib/installer/installer.go | 237 +++++++++--- .../aitools/lib/installer/installer_test.go | 366 ++++++++++++++++++ 4 files changed, 763 insertions(+), 96 deletions(-) diff --git a/experimental/aitools/cmd/install_test.go b/experimental/aitools/cmd/install_test.go index 64f97ddf9c..8e88f32342 100644 --- a/experimental/aitools/cmd/install_test.go +++ b/experimental/aitools/cmd/install_test.go @@ -2,74 +2,178 @@ package aitools import ( "context" + "bufio" + "os" + "path/filepath" "testing" + "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/experimental/aitools/lib/installer" + "github.com/databricks/cli/libs/cmdio" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func setupInstallMock(t *testing.T) *[]installCall { + t.Helper() + orig := installSkillsForAgentsFn + t.Cleanup(func() { installSkillsForAgentsFn = orig }) + + var calls []installCall + installSkillsForAgentsFn = func(_ context.Context, _ installer.ManifestSource, targetAgents []*agents.Agent, opts installer.InstallOptions) error { + names := make([]string, len(targetAgents)) + for i, a := range targetAgents { + names[i] = a.Name + } + calls = append(calls, installCall{agents: names, opts: opts}) + return nil + } + return &calls +} + +type installCall struct { + agents []string + opts installer.InstallOptions +} + +func setupTestAgents(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + t.Setenv("HOME", tmp) + // Create config dirs for two agents. + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".cursor"), 0o755)) + return tmp +} + func TestInstallCommandsDelegateToSkillsInstall(t *testing.T) { - originalInstallAllSkills := installAllSkills - originalInstallSkill := installSkill - t.Cleanup(func() { - installAllSkills = originalInstallAllSkills - installSkill = originalInstallSkill - }) + setupTestAgents(t) + calls := setupInstallMock(t) tests := []struct { - name string - newCmd func() *cobra.Command - args []string - wantAllCalls int - wantSkillCalls []string + name string + newCmd func() *cobra.Command + args []string + wantAgents int + wantSkills []string }{ { - name: "skills install installs all skills", - newCmd: newSkillsInstallCmd, - wantAllCalls: 1, + name: "skills install installs all skills for all agents", + newCmd: newSkillsInstallCmd, + wantAgents: 2, }, { - name: "skills install forwards skill name", - newCmd: newSkillsInstallCmd, - args: []string{"bundle/review"}, - wantSkillCalls: []string{"bundle/review"}, + name: "skills install forwards skill name", + newCmd: newSkillsInstallCmd, + args: []string{"bundle/review"}, + wantAgents: 2, + wantSkills: []string{"bundle/review"}, }, { - name: "top level install installs all skills", - newCmd: newInstallCmd, - wantAllCalls: 1, + name: "top level install installs all skills", + newCmd: newInstallCmd, + wantAgents: 2, }, { - name: "top level install forwards skill name", - newCmd: newInstallCmd, - args: []string{"bundle/review"}, - wantSkillCalls: []string{"bundle/review"}, + name: "top level install forwards skill name", + newCmd: newInstallCmd, + args: []string{"bundle/review"}, + wantAgents: 2, + wantSkills: []string{"bundle/review"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - allCalls := 0 - var skillCalls []string - - installAllSkills = func(context.Context) error { - allCalls++ - return nil - } - installSkill = func(_ context.Context, skillName string) error { - skillCalls = append(skillCalls, skillName) - return nil - } + *calls = nil + ctx := cmdio.MockDiscard(t.Context()) cmd := tt.newCmd() - cmd.SetContext(t.Context()) + cmd.SetContext(ctx) err := cmd.RunE(cmd, tt.args) require.NoError(t, err) - assert.Equal(t, tt.wantAllCalls, allCalls) - assert.Equal(t, tt.wantSkillCalls, skillCalls) + require.Len(t, *calls, 1) + assert.Len(t, (*calls)[0].agents, tt.wantAgents) + assert.Equal(t, tt.wantSkills, (*calls)[0].opts.SpecificSkills) }) } } + +func TestRunSkillsInstallInteractivePrompt(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + origPrompt := promptAgentSelection + t.Cleanup(func() { promptAgentSelection = origPrompt }) + + promptCalled := false + promptAgentSelection = func(_ context.Context, detected []*agents.Agent) ([]*agents.Agent, error) { + promptCalled = true + // Return only the first agent. + return detected[:1], nil + } + + // Use SetupTest with PromptSupported=true to simulate interactive terminal. + ctx, test := cmdio.SetupTest(t.Context(), cmdio.TestOptions{PromptSupported: true}) + defer test.Done() + + // Drain both pipes in background to prevent blocking. + drain := func(r *bufio.Reader) { + buf := make([]byte, 4096) + for { + _, err := r.Read(buf) + if err != nil { + return + } + } + } + go drain(test.Stdout) + go drain(test.Stderr) + + err := runSkillsInstall(ctx, nil) + require.NoError(t, err) + + assert.True(t, promptCalled, "prompt should be called when 2+ agents detected and interactive") + require.Len(t, *calls, 1) + assert.Len(t, (*calls)[0].agents, 1, "only the selected agent should be passed") +} + +func TestRunSkillsInstallNonInteractiveUsesAllAgents(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + origPrompt := promptAgentSelection + t.Cleanup(func() { promptAgentSelection = origPrompt }) + + promptCalled := false + promptAgentSelection = func(_ context.Context, detected []*agents.Agent) ([]*agents.Agent, error) { + promptCalled = true + return detected, nil + } + + // MockDiscard gives a non-interactive context. + ctx := cmdio.MockDiscard(t.Context()) + + err := runSkillsInstall(ctx, nil) + require.NoError(t, err) + + assert.False(t, promptCalled, "prompt should not be called in non-interactive mode") + require.Len(t, *calls, 1) + assert.Len(t, (*calls)[0].agents, 2, "all detected agents should be used") +} + +func TestRunSkillsInstallNoAgents(t *testing.T) { + // Set HOME to empty dir so no agents are detected. + tmp := t.TempDir() + t.Setenv("HOME", tmp) + + calls := setupInstallMock(t) + ctx := cmdio.MockDiscard(t.Context()) + + err := runSkillsInstall(ctx, nil) + require.NoError(t, err) + assert.Empty(t, *calls, "install should not be called when no agents detected") +} diff --git a/experimental/aitools/cmd/skills.go b/experimental/aitools/cmd/skills.go index 325ee3c1c0..e29910e57c 100644 --- a/experimental/aitools/cmd/skills.go +++ b/experimental/aitools/cmd/skills.go @@ -2,16 +2,51 @@ package aitools import ( "context" - + "fmt" + "github.com/charmbracelet/huh" + "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/experimental/aitools/lib/installer" + "github.com/databricks/cli/libs/cmdio" + "github.com/fatih/color" "github.com/spf13/cobra" ) +// Package-level vars for testability. var ( - installAllSkills = installer.InstallAllSkills - installSkill = installer.InstallSkill + promptAgentSelection = defaultPromptAgentSelection + installSkillsForAgentsFn = installer.InstallSkillsForAgents ) +func defaultPromptAgentSelection(ctx context.Context, detected []*agents.Agent) ([]*agents.Agent, error) { + options := make([]huh.Option[string], 0, len(detected)) + agentsByName := make(map[string]*agents.Agent, len(detected)) + for _, a := range detected { + options = append(options, huh.NewOption(a.DisplayName, a.Name).Selected(true)) + agentsByName[a.Name] = a + } + + var selected []string + err := huh.NewMultiSelect[string](). + Title("Select coding agents to install skills for"). + Description("space to toggle, enter to confirm"). + Options(options...). + Value(&selected). + Run() + if err != nil { + return nil, err + } + + if len(selected) == 0 { + return nil, fmt.Errorf("at least one agent must be selected") + } + + result := make([]*agents.Agent, 0, len(selected)) + for _, name := range selected { + result = append(result, agentsByName[name]) + } + return result, nil +} + func newSkillsCmd() *cobra.Command { cmd := &cobra.Command{ Use: "skills", @@ -53,9 +88,38 @@ Supported agents: Claude Code, Cursor, Codex CLI, OpenCode, GitHub Copilot, Anti } func runSkillsInstall(ctx context.Context, args []string) error { + detected := agents.DetectInstalled(ctx) + if len(detected) == 0 { + cmdio.LogString(ctx, color.YellowString("No supported coding agents detected.")) + cmdio.LogString(ctx, "") + cmdio.LogString(ctx, "Supported agents: Claude Code, Cursor, Codex CLI, OpenCode, GitHub Copilot, Antigravity") + cmdio.LogString(ctx, "Please install at least one coding agent first.") + return nil + } + + var targetAgents []*agents.Agent + switch { + case len(detected) == 1: + targetAgents = detected + case cmdio.IsPromptSupported(ctx): + var err error + targetAgents, err = promptAgentSelection(ctx, detected) + if err != nil { + return err + } + default: + // Non-interactive: install for all detected agents. + targetAgents = detected + } + + installer.PrintInstallingFor(ctx, targetAgents) + + opts := installer.InstallOptions{} if len(args) > 0 { - return installSkill(ctx, args[0]) + opts.SpecificSkills = []string{args[0]} } - return installAllSkills(ctx) + src := &installer.GitHubManifestSource{} + return installSkillsForAgentsFn(ctx, src, targetAgents, opts) } + diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index b9f2d7b899..18e8c6f1e0 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -7,12 +7,16 @@ import ( "net/http" "os" "path/filepath" + "strings" "time" "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/log" "github.com/fatih/color" + "golang.org/x/mod/semver" ) const ( @@ -22,6 +26,10 @@ const ( defaultSkillsRepoRef = "v0.1.3" ) +// fetchFileFn is the function used to download individual skill files. +// It is a package-level var so tests can replace it with a mock. +var fetchFileFn = fetchSkillFile + func getSkillsRef(ctx context.Context) string { if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" { return ref @@ -46,6 +54,12 @@ type SkillMeta struct { MinCLIVer string `json:"min_cli_version,omitempty"` } +// InstallOptions controls the behavior of InstallSkillsForAgents. +type InstallOptions struct { + IncludeExperimental bool + SpecificSkills []string // empty = all skills +} + // FetchManifest fetches the skills manifest from the skills repo. // This is a convenience wrapper that uses the default GitHubManifestSource. func FetchManifest(ctx context.Context) (*Manifest, error) { @@ -54,9 +68,9 @@ func FetchManifest(ctx context.Context) (*Manifest, error) { return src.FetchManifest(ctx, ref) } -func fetchSkillFile(ctx context.Context, skillName, filePath string) ([]byte, error) { +func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byte, error) { url := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s/%s/%s", - skillsRepoOwner, skillsRepoName, getSkillsRef(ctx), skillsRepoPath, skillName, filePath) + skillsRepoOwner, skillsRepoName, ref, skillsRepoPath, skillName, filePath) req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { @@ -97,49 +111,157 @@ func ListSkills(ctx context.Context) error { return nil } -// InstallAllSkills fetches the manifest and installs all skills for detected agents. -func InstallAllSkills(ctx context.Context) error { - manifest, err := FetchManifest(ctx) +// InstallSkillsForAgents fetches the manifest and installs skills for the given agents. +// This is the core installation function. Callers are responsible for agent detection, +// prompting, and printing the "Installing..." header. +func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgents []*agents.Agent, opts InstallOptions) error { + latestTag, err := src.FetchLatestRelease(ctx) + if err != nil { + return fmt.Errorf("failed to fetch latest release: %w", err) + } + + manifest, err := src.FetchManifest(ctx, latestTag) if err != nil { return err } - detectedAgents := agents.DetectInstalled(ctx) - if len(detectedAgents) == 0 { - printNoAgentsDetected(ctx) - return nil + globalDir, err := GlobalSkillsDir(ctx) + if err != nil { + return err } - printDetectedAgents(ctx, detectedAgents) + // Load existing state for idempotency checks. + state, err := LoadState(globalDir) + if err != nil { + return fmt.Errorf("failed to load install state: %w", err) + } - for name, meta := range manifest.Skills { - if err := installSkillForAgents(ctx, name, meta.Files, detectedAgents); err != nil { + // Detect legacy installs (skills on disk but no state file). + if state == nil { + checkLegacyInstall(ctx, globalDir) + } + + // Filter skills based on options, experimental flag, and CLI version. + targetSkills, err := resolveSkills(ctx, manifest.Skills, opts) + if err != nil { + return err + } + + // Install each skill. + for name, meta := range targetSkills { + // Idempotency: skip if same version is already installed and on disk. + if state != nil && state.Skills[name] == meta.Version { + skillDir := filepath.Join(globalDir, name) + if _, statErr := os.Stat(skillDir); statErr == nil { + log.Debugf(ctx, "%s v%s already installed, skipping", name, meta.Version) + continue + } + } + + if err := installSkillForAgents(ctx, latestTag, name, meta.Files, targetAgents, globalDir); err != nil { return err } } + + // Save state. + newState := &InstallState{ + SchemaVersion: 1, + IncludeExperimental: opts.IncludeExperimental, + Release: latestTag, + LastUpdated: time.Now(), + Skills: make(map[string]string, len(targetSkills)), + } + for name, meta := range targetSkills { + newState.Skills[name] = meta.Version + } + if err := SaveState(globalDir, newState); err != nil { + return err + } + + tag := strings.TrimPrefix(latestTag, "v") + cmdio.LogString(ctx, fmt.Sprintf("Installed %d skills (v%s).", len(targetSkills), tag)) return nil } -// InstallSkill fetches the manifest and installs a single skill by name. -func InstallSkill(ctx context.Context, skillName string) error { - manifest, err := FetchManifest(ctx) - if err != nil { - return err +// resolveSkills filters the manifest skills based on the install options, +// experimental flag, and CLI version constraints. +func resolveSkills(ctx context.Context, skills map[string]SkillMeta, opts InstallOptions) (map[string]SkillMeta, error) { + isSpecific := len(opts.SpecificSkills) > 0 + cliVersion := build.GetInfo().Version + isDev := strings.HasPrefix(cliVersion, build.DefaultSemver) + + // Start with all skills or only the requested ones. + var candidates map[string]SkillMeta + if isSpecific { + candidates = make(map[string]SkillMeta, len(opts.SpecificSkills)) + for _, name := range opts.SpecificSkills { + meta, ok := skills[name] + if !ok { + return nil, fmt.Errorf("skill %q not found", name) + } + candidates[name] = meta + } + } else { + candidates = skills } - if _, ok := manifest.Skills[skillName]; !ok { - return fmt.Errorf("skill %q not found", skillName) + result := make(map[string]SkillMeta, len(candidates)) + for name, meta := range candidates { + if meta.Experimental && !opts.IncludeExperimental { + if isSpecific { + return nil, fmt.Errorf("skill %q is experimental; use --experimental to install", name) + } + log.Debugf(ctx, "Skipping experimental skill %s", name) + continue + } + + if meta.MinCLIVer != "" && !isDev && semver.Compare("v"+cliVersion, "v"+meta.MinCLIVer) < 0 { + if isSpecific { + return nil, fmt.Errorf("skill %q requires CLI version %s (running %s)", name, meta.MinCLIVer, cliVersion) + } + log.Warnf(ctx, "Skipping %s: requires CLI version %s (running %s)", name, meta.MinCLIVer, cliVersion) + continue + } + + result[name] = meta } + return result, nil +} - detectedAgents := agents.DetectInstalled(ctx) - if len(detectedAgents) == 0 { +// InstallAllSkills fetches the manifest and installs all skills for detected agents. +// The signature is func(context.Context) error to satisfy the callback in cmd/apps/init.go. +func InstallAllSkills(ctx context.Context) error { + installed := agents.DetectInstalled(ctx) + if len(installed) == 0 { printNoAgentsDetected(ctx) return nil } - printDetectedAgents(ctx, detectedAgents) + PrintInstallingFor(ctx, installed) + src := &GitHubManifestSource{} + return InstallSkillsForAgents(ctx, src, installed, InstallOptions{}) +} - return installSkillForAgents(ctx, skillName, manifest.Skills[skillName].Files, detectedAgents) +// InstallSkill installs a single skill by name for all detected agents. +func InstallSkill(ctx context.Context, skillName string) error { + installed := agents.DetectInstalled(ctx) + if len(installed) == 0 { + printNoAgentsDetected(ctx) + return nil + } + + PrintInstallingFor(ctx, installed) + src := &GitHubManifestSource{} + return InstallSkillsForAgents(ctx, src, installed, InstallOptions{SpecificSkills: []string{skillName}}) +} + +// PrintInstallingFor prints the "Installing..." header with agent names. +func PrintInstallingFor(ctx context.Context, targetAgents []*agents.Agent) { + names := make([]string, len(targetAgents)) + for i, a := range targetAgents { + names[i] = a.DisplayName + } + cmdio.LogString(ctx, fmt.Sprintf("Installing Databricks AI skills for %s...", strings.Join(names, ", "))) } func printNoAgentsDetected(ctx context.Context) { @@ -149,61 +271,73 @@ func printNoAgentsDetected(ctx context.Context) { cmdio.LogString(ctx, "Please install at least one coding agent first.") } -func printDetectedAgents(ctx context.Context, detectedAgents []*agents.Agent) { - cmdio.LogString(ctx, "Detected coding agents:") - for _, agent := range detectedAgents { - cmdio.LogString(ctx, " - "+agent.DisplayName) +// checkLegacyInstall prints a message if skills exist on disk but no state file was found. +func checkLegacyInstall(ctx context.Context, globalDir string) { + if hasSkillsOnDisk(globalDir) { + cmdio.LogString(ctx, "Found skills installed before state tracking was added. Run 'databricks experimental aitools install' to refresh.") + return + } + homeDir, err := env.UserHomeDir(ctx) + if err != nil { + return + } + legacyDir := filepath.Join(homeDir, ".databricks", "agent-skills") + if hasSkillsOnDisk(legacyDir) { + cmdio.LogString(ctx, "Found skills installed before state tracking was added. Run 'databricks experimental aitools install' to refresh.") } - cmdio.LogString(ctx, "") } -func installSkillForAgents(ctx context.Context, skillName string, files []string, detectedAgents []*agents.Agent) error { - homeDir, err := env.UserHomeDir(ctx) +// hasSkillsOnDisk checks if a directory contains subdirectories starting with "databricks". +func hasSkillsOnDisk(dir string) bool { + entries, err := os.ReadDir(dir) if err != nil { - return fmt.Errorf("failed to get home directory: %w", err) + return false + } + for _, e := range entries { + if e.IsDir() && strings.HasPrefix(e.Name(), "databricks") { + return true + } } + return false +} - // Always install to canonical location first. - canonicalDir := filepath.Join(homeDir, agents.CanonicalSkillsDir, skillName) - if err := installSkillToDir(ctx, skillName, canonicalDir, files); err != nil { +func installSkillForAgents(ctx context.Context, ref, skillName string, files []string, detectedAgents []*agents.Agent, globalDir string) error { + canonicalDir := filepath.Join(globalDir, skillName) + if err := installSkillToDir(ctx, ref, skillName, canonicalDir, files); err != nil { return err } useSymlinks := len(detectedAgents) > 1 - // install/symlink to each agent for _, agent := range detectedAgents { agentSkillDir, err := agent.SkillsDir(ctx) if err != nil { - cmdio.LogString(ctx, color.YellowString("⊘ Skipped %s: %v", agent.DisplayName, err)) + log.Warnf(ctx, "Skipped %s: %v", agent.DisplayName, err) continue } destDir := filepath.Join(agentSkillDir, skillName) - // Back up existing non-canonical skills before overwriting. if err := backupThirdPartySkill(ctx, destDir, canonicalDir, skillName, agent.DisplayName); err != nil { - cmdio.LogString(ctx, color.YellowString("⊘ Failed to back up existing skill for %s: %v", agent.DisplayName, err)) + log.Warnf(ctx, "Failed to back up existing skill for %s: %v", agent.DisplayName, err) continue } if useSymlinks { if err := createSymlink(canonicalDir, destDir); err != nil { - // fallback to copy on symlink failure (e.g., Windows without admin) - cmdio.LogString(ctx, color.YellowString(" Symlink failed for %s, copying instead...", agent.DisplayName)) - if err := installSkillToDir(ctx, skillName, destDir, files); err != nil { - cmdio.LogString(ctx, color.YellowString("⊘ Failed to install for %s: %v", agent.DisplayName, err)) + log.Debugf(ctx, "Symlink failed for %s, copying instead: %v", agent.DisplayName, err) + if err := installSkillToDir(ctx, ref, skillName, destDir, files); err != nil { + log.Warnf(ctx, "Failed to install for %s: %v", agent.DisplayName, err) continue } } - cmdio.LogString(ctx, color.GreenString("✓ Installed %q for %s (symlinked)", skillName, agent.DisplayName)) + log.Debugf(ctx, "Installed %q for %s (symlinked)", skillName, agent.DisplayName) } else { - // single agent - copy from canonical - if err := installSkillToDir(ctx, skillName, destDir, files); err != nil { - cmdio.LogString(ctx, color.YellowString("⊘ Failed to install for %s: %v", agent.DisplayName, err)) + if err := installSkillToDir(ctx, ref, skillName, destDir, files); err != nil { + log.Warnf(ctx, "Failed to install for %s: %v", agent.DisplayName, err) continue } - cmdio.LogString(ctx, color.GreenString("✓ Installed %q for %s", skillName, agent.DisplayName)) + log.Debugf(ctx, "Installed %q for %s", skillName, agent.DisplayName) } } @@ -239,11 +373,11 @@ func backupThirdPartySkill(ctx context.Context, destDir, canonicalDir, skillName return fmt.Errorf("failed to move existing skill: %w", err) } - cmdio.LogString(ctx, color.YellowString(" Existing %q for %s moved to %s", skillName, agentName, backupDest)) + log.Debugf(ctx, "Existing %q for %s moved to %s", skillName, agentName, backupDest) return nil } -func installSkillToDir(ctx context.Context, skillName, destDir string, files []string) error { +func installSkillToDir(ctx context.Context, ref, skillName, destDir string, files []string) error { // remove existing skill directory for clean install if err := os.RemoveAll(destDir); err != nil { return fmt.Errorf("failed to remove existing skill: %w", err) @@ -253,20 +387,19 @@ func installSkillToDir(ctx context.Context, skillName, destDir string, files []s return fmt.Errorf("failed to create directory: %w", err) } - // download all files for _, file := range files { - content, err := fetchSkillFile(ctx, skillName, file) + content, err := fetchFileFn(ctx, ref, skillName, file) if err != nil { return err } destPath := filepath.Join(destDir, file) - // create parent directories if needed if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { return fmt.Errorf("failed to create directory: %w", err) } + log.Debugf(ctx, "Downloading %s/%s", skillName, file) if err := os.WriteFile(destPath, content, 0o644); err != nil { return fmt.Errorf("failed to write %s: %w", file, err) } diff --git a/experimental/aitools/lib/installer/installer_test.go b/experimental/aitools/lib/installer/installer_test.go index 5caa1e3b57..1636c0ff30 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -1,15 +1,90 @@ package installer import ( + "context" "os" "path/filepath" "testing" + "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/cmdio" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// mockManifestSource is a test double for ManifestSource. +type mockManifestSource struct { + manifest *Manifest + release string + fetchErr error +} + +func (m *mockManifestSource) FetchManifest(_ context.Context, _ string) (*Manifest, error) { + if m.fetchErr != nil { + return nil, m.fetchErr + } + return m.manifest, nil +} + +func (m *mockManifestSource) FetchLatestRelease(_ context.Context) (string, error) { + return m.release, nil +} + +func testManifest() *Manifest { + return &Manifest{ + Version: "1", + UpdatedAt: "2024-01-01", + Skills: map[string]SkillMeta{ + "databricks-sql": { + Version: "0.1.0", + Files: []string{"SKILL.md"}, + }, + "databricks-jobs": { + Version: "0.1.0", + Files: []string{"SKILL.md"}, + }, + }, + } +} + +func setupFetchMock(t *testing.T) { + t.Helper() + orig := fetchFileFn + t.Cleanup(func() { fetchFileFn = orig }) + fetchFileFn = func(_ context.Context, _, skillName, filePath string) ([]byte, error) { + return []byte("# " + skillName + "/" + filePath), nil + } +} + +func testAgent(tmpHome string) *agents.Agent { + return &agents.Agent{ + Name: "test-agent", + DisplayName: "Test Agent", + ConfigDir: func(_ context.Context) (string, error) { + return filepath.Join(tmpHome, ".test-agent"), nil + }, + } +} + +func setupTestHome(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + t.Setenv("HOME", tmp) + // Create agent config dir so the agent is "detected". + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".test-agent"), 0o755)) + return tmp +} + +func setBuildVersion(t *testing.T, version string) { + t.Helper() + orig := build.GetInfo().Version + build.SetBuildVersion(version) + t.Cleanup(func() { build.SetBuildVersion(orig) }) +} + +// --- Backup tests (unchanged from PR 1) --- + func TestBackupThirdPartySkillDestDoesNotExist(t *testing.T) { ctx := cmdio.MockDiscard(t.Context()) destDir := filepath.Join(t.TempDir(), "nonexistent") @@ -109,3 +184,294 @@ func TestBackupThirdPartySkillRegularFile(t *testing.T) { _, err = os.Stat(destDir) assert.True(t, os.IsNotExist(err)) } + +// --- InstallSkillsForAgents tests --- + +func TestInstallSkillsForAgentsWritesState(t *testing.T) { + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) + require.NoError(t, err) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + require.NotNil(t, state) + assert.Equal(t, 1, state.SchemaVersion) + assert.Equal(t, "v0.1.0", state.Release) + assert.Len(t, state.Skills, 2) + assert.Equal(t, "0.1.0", state.Skills["databricks-sql"]) + assert.Equal(t, "0.1.0", state.Skills["databricks-jobs"]) + + assert.Contains(t, stderr.String(), "Installed 2 skills (v0.1.0).") +} + +func TestInstallSkillForSingleWritesState(t *testing.T) { + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{ + SpecificSkills: []string{"databricks-sql"}, + }) + require.NoError(t, err) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + require.NotNil(t, state) + assert.Len(t, state.Skills, 1) + assert.Equal(t, "0.1.0", state.Skills["databricks-sql"]) + + assert.Contains(t, stderr.String(), "Installed 1 skills (v0.1.0).") +} + +func TestInstallSkillsSpecificNotFound(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{ + SpecificSkills: []string{"nonexistent"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), `skill "nonexistent" not found`) +} + +func TestExperimentalSkillsSkippedByDefault(t *testing.T) { + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + + manifest := testManifest() + manifest.Skills["databricks-experimental"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + Experimental: true, + } + + src := &mockManifestSource{manifest: manifest, release: "v0.1.0"} + agent := testAgent(tmp) + + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) + require.NoError(t, err) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + // Only non-experimental skills should be installed. + assert.Len(t, state.Skills, 2) + assert.NotContains(t, state.Skills, "databricks-experimental") + + assert.Contains(t, stderr.String(), "Installed 2 skills (v0.1.0).") +} + +func TestExperimentalSkillsIncludedWithFlag(t *testing.T) { + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + + manifest := testManifest() + manifest.Skills["databricks-experimental"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + Experimental: true, + } + + src := &mockManifestSource{manifest: manifest, release: "v0.1.0"} + agent := testAgent(tmp) + + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{ + IncludeExperimental: true, + }) + require.NoError(t, err) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.Len(t, state.Skills, 3) + assert.Contains(t, state.Skills, "databricks-experimental") + assert.True(t, state.IncludeExperimental) + + assert.Contains(t, stderr.String(), "Installed 3 skills (v0.1.0).") +} + +func TestMinCLIVersionSkipWithWarningForInstallAll(t *testing.T) { + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + setBuildVersion(t, "0.200.0") + + manifest := testManifest() + manifest.Skills["databricks-future"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + MinCLIVer: "0.300.0", + } + + src := &mockManifestSource{manifest: manifest, release: "v0.1.0"} + agent := testAgent(tmp) + + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) + require.NoError(t, err) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + // The high-version skill should be skipped. + assert.Len(t, state.Skills, 2) + assert.NotContains(t, state.Skills, "databricks-future") + + assert.Contains(t, stderr.String(), "Installed 2 skills (v0.1.0).") +} + +func TestMinCLIVersionHardErrorForInstallSingle(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + setBuildVersion(t, "0.200.0") + + manifest := testManifest() + manifest.Skills["databricks-future"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + MinCLIVer: "0.300.0", + } + + src := &mockManifestSource{manifest: manifest, release: "v0.1.0"} + agent := testAgent(tmp) + + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{ + SpecificSkills: []string{"databricks-future"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "requires CLI version 0.300.0") + assert.Contains(t, err.Error(), "running 0.200.0") +} + +func TestIdempotentSecondInstallSkips(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + + // First install. + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) + require.NoError(t, err) + + // Track fetch calls on second install. + fetchCalls := 0 + orig := fetchFileFn + t.Cleanup(func() { fetchFileFn = orig }) + fetchFileFn = func(_ context.Context, _, skillName, filePath string) ([]byte, error) { + fetchCalls++ + return []byte("# " + skillName + "/" + filePath), nil + } + + // Second install with same version. + err = InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) + require.NoError(t, err) + + // No files should be fetched since everything is up to date. + assert.Equal(t, 0, fetchCalls) +} + +func TestIdempotentInstallUpdatesNewVersions(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + + // First install. + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) + require.NoError(t, err) + + // Update manifest with a new version for one skill. + updatedManifest := testManifest() + updatedManifest.Skills["databricks-sql"] = SkillMeta{ + Version: "0.2.0", + Files: []string{"SKILL.md"}, + } + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + + // Track which skills are fetched. + var fetchedSkills []string + orig := fetchFileFn + t.Cleanup(func() { fetchFileFn = orig }) + fetchFileFn = func(_ context.Context, _, skillName, filePath string) ([]byte, error) { + fetchedSkills = append(fetchedSkills, skillName) + return []byte("# " + skillName + "/" + filePath), nil + } + + // Second install with updated manifest. + err = InstallSkillsForAgents(ctx, src2, []*agents.Agent{agent}, InstallOptions{}) + require.NoError(t, err) + + // Both skills should be fetched because the release tag changed. + // (databricks-sql has a new version, databricks-jobs has the same version + // but state was from v0.1.0 release.) + assert.Contains(t, fetchedSkills, "databricks-sql") + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.Equal(t, "v0.2.0", state.Release) + assert.Equal(t, "0.2.0", state.Skills["databricks-sql"]) +} + +func TestLegacyDetectMessagePrinted(t *testing.T) { + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + + // Create skills on disk at canonical location but no state file. + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + require.NoError(t, os.MkdirAll(filepath.Join(globalDir, "databricks-sql"), 0o755)) + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) + require.NoError(t, err) + + assert.Contains(t, stderr.String(), "Found skills installed before state tracking was added.") +} + +func TestLegacyDetectLegacyDir(t *testing.T) { + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + + // Create skills in the legacy location. + legacyDir := filepath.Join(tmp, ".databricks", "agent-skills") + require.NoError(t, os.MkdirAll(filepath.Join(legacyDir, "databricks-sql"), 0o755)) + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) + require.NoError(t, err) + + assert.Contains(t, stderr.String(), "Found skills installed before state tracking was added.") +} + +func TestInstallAllSkillsSignaturePreserved(t *testing.T) { + // Compile-time check that InstallAllSkills satisfies func(context.Context) error. + var fn func(context.Context) error = InstallAllSkills + _ = fn +} From 21714a00da0b7ba5170cf7a213f1428ffb9abb4e Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 09:07:44 +0100 Subject: [PATCH 04/12] Fix review findings: flag name, state merge, log level, grammar, test assertion - Fix --experimental -> --include-experimental in error message - Merge new skills into existing state instead of overwriting - Move FetchManifest log from Info to Debug for concise output - Handle singular/plural in Installed N skill(s) message - Assert min_cli_version skip warning appears in test output Co-authored-by: Isaac --- .../aitools/lib/installer/installer.go | 41 +++++++++++++------ .../aitools/lib/installer/installer_test.go | 11 ++++- experimental/aitools/lib/installer/source.go | 2 +- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 18e8c6f1e0..ab1decba2c 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -163,23 +163,40 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent } } - // Save state. - newState := &InstallState{ - SchemaVersion: 1, - IncludeExperimental: opts.IncludeExperimental, - Release: latestTag, - LastUpdated: time.Now(), - Skills: make(map[string]string, len(targetSkills)), - } - for name, meta := range targetSkills { - newState.Skills[name] = meta.Version + // Save state. Merge into existing state so skills from previous installs + // (e.g., experimental skills from a prior run) are preserved. + existingState, _ := LoadState(globalDir) + var newState *InstallState + if existingState != nil { + newState = existingState + newState.Release = latestTag + newState.LastUpdated = time.Now() + newState.IncludeExperimental = opts.IncludeExperimental + for name, meta := range targetSkills { + newState.Skills[name] = meta.Version + } + } else { + newState = &InstallState{ + SchemaVersion: 1, + IncludeExperimental: opts.IncludeExperimental, + Release: latestTag, + LastUpdated: time.Now(), + Skills: make(map[string]string, len(targetSkills)), + } + for name, meta := range targetSkills { + newState.Skills[name] = meta.Version + } } if err := SaveState(globalDir, newState); err != nil { return err } tag := strings.TrimPrefix(latestTag, "v") - cmdio.LogString(ctx, fmt.Sprintf("Installed %d skills (v%s).", len(targetSkills), tag)) + noun := "skills" + if len(targetSkills) == 1 { + noun = "skill" + } + cmdio.LogString(ctx, fmt.Sprintf("Installed %d %s (v%s).", len(targetSkills), noun, tag)) return nil } @@ -209,7 +226,7 @@ func resolveSkills(ctx context.Context, skills map[string]SkillMeta, opts Instal for name, meta := range candidates { if meta.Experimental && !opts.IncludeExperimental { if isSpecific { - return nil, fmt.Errorf("skill %q is experimental; use --experimental to install", name) + return nil, fmt.Errorf("skill %q is experimental; use --include-experimental to install", name) } log.Debugf(ctx, "Skipping experimental skill %s", name) continue diff --git a/experimental/aitools/lib/installer/installer_test.go b/experimental/aitools/lib/installer/installer_test.go index 1636c0ff30..7d9ebc95a3 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -1,7 +1,9 @@ package installer import ( + "bytes" "context" + "log/slog" "os" "path/filepath" "testing" @@ -9,6 +11,7 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -231,7 +234,7 @@ func TestInstallSkillForSingleWritesState(t *testing.T) { assert.Len(t, state.Skills, 1) assert.Equal(t, "0.1.0", state.Skills["databricks-sql"]) - assert.Contains(t, stderr.String(), "Installed 1 skills (v0.1.0).") + assert.Contains(t, stderr.String(), "Installed 1 skill (v0.1.0).") } func TestInstallSkillsSpecificNotFound(t *testing.T) { @@ -313,6 +316,11 @@ func TestMinCLIVersionSkipWithWarningForInstallAll(t *testing.T) { setupFetchMock(t) setBuildVersion(t, "0.200.0") + // Capture log output to verify the warning. + var logBuf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelWarn})) + ctx = log.NewContext(ctx, logger) + manifest := testManifest() manifest.Skills["databricks-future"] = SkillMeta{ Version: "0.1.0", @@ -334,6 +342,7 @@ func TestMinCLIVersionSkipWithWarningForInstallAll(t *testing.T) { assert.NotContains(t, state.Skills, "databricks-future") assert.Contains(t, stderr.String(), "Installed 2 skills (v0.1.0).") + assert.Contains(t, logBuf.String(), "requires CLI version 0.300.0") } func TestMinCLIVersionHardErrorForInstallSingle(t *testing.T) { diff --git a/experimental/aitools/lib/installer/source.go b/experimental/aitools/lib/installer/source.go index c32eb721d8..59039b8b80 100644 --- a/experimental/aitools/lib/installer/source.go +++ b/experimental/aitools/lib/installer/source.go @@ -29,7 +29,7 @@ type GitHubManifestSource struct{} // FetchManifest fetches the skills manifest from GitHub at the given ref. func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (*Manifest, error) { - log.Infof(ctx, "Fetching skills manifest from %s/%s@%s", skillsRepoOwner, skillsRepoName, ref) + log.Debugf(ctx, "Fetching skills manifest from %s/%s@%s", skillsRepoOwner, skillsRepoName, ref) url := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/manifest.json", skillsRepoOwner, skillsRepoName, ref) From 35232b5eabc7b3e5d7f85922de5e448fd8d31514 Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 09:08:37 +0100 Subject: [PATCH 05/12] Fix trailing whitespace in skills.go --- experimental/aitools/cmd/skills.go | 1 - 1 file changed, 1 deletion(-) diff --git a/experimental/aitools/cmd/skills.go b/experimental/aitools/cmd/skills.go index e29910e57c..9550eb6b33 100644 --- a/experimental/aitools/cmd/skills.go +++ b/experimental/aitools/cmd/skills.go @@ -122,4 +122,3 @@ func runSkillsInstall(ctx context.Context, args []string) error { src := &installer.GitHubManifestSource{} return installSkillsForAgentsFn(ctx, src, targetAgents, opts) } - From dfcbe18f5b6f768932192dfa6261d84d3a09c9cb Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 09:16:56 +0100 Subject: [PATCH 06/12] Add update, uninstall, and version commands for aitools skills Adds three new commands to the aitools skill management: - `update`: Updates installed skills to latest release with --check (dry run), --force, and --no-new flags. Auto-adds new manifest skills by default. - `uninstall`: Removes all installed skills, symlinks from all registry agents, cleans orphaned symlinks, and deletes state file. - `version`: Shows installed version, skill count, staleness check against latest release. Co-authored-by: Isaac --- experimental/aitools/cmd/aitools.go | 3 + experimental/aitools/cmd/uninstall.go | 19 ++ experimental/aitools/cmd/update.go | 44 +++ experimental/aitools/cmd/version.go | 68 ++++ .../aitools/lib/installer/uninstall.go | 147 +++++++++ .../aitools/lib/installer/uninstall_test.go | 194 ++++++++++++ experimental/aitools/lib/installer/update.go | 223 +++++++++++++ .../aitools/lib/installer/update_test.go | 292 ++++++++++++++++++ 8 files changed, 990 insertions(+) create mode 100644 experimental/aitools/cmd/uninstall.go create mode 100644 experimental/aitools/cmd/update.go create mode 100644 experimental/aitools/cmd/version.go create mode 100644 experimental/aitools/lib/installer/uninstall.go create mode 100644 experimental/aitools/lib/installer/uninstall_test.go create mode 100644 experimental/aitools/lib/installer/update.go create mode 100644 experimental/aitools/lib/installer/update_test.go diff --git a/experimental/aitools/cmd/aitools.go b/experimental/aitools/cmd/aitools.go index 3058013dbe..3ce43a1073 100644 --- a/experimental/aitools/cmd/aitools.go +++ b/experimental/aitools/cmd/aitools.go @@ -20,6 +20,9 @@ Provides commands to: cmd.AddCommand(newInstallCmd()) cmd.AddCommand(newSkillsCmd()) cmd.AddCommand(newToolsCmd()) + cmd.AddCommand(newUpdateCmd()) + cmd.AddCommand(newUninstallCmd()) + cmd.AddCommand(newVersionCmd()) return cmd } diff --git a/experimental/aitools/cmd/uninstall.go b/experimental/aitools/cmd/uninstall.go new file mode 100644 index 0000000000..9edd360100 --- /dev/null +++ b/experimental/aitools/cmd/uninstall.go @@ -0,0 +1,19 @@ +package aitools + +import ( + "github.com/databricks/cli/experimental/aitools/lib/installer" + "github.com/spf13/cobra" +) + +func newUninstallCmd() *cobra.Command { + return &cobra.Command{ + Use: "uninstall", + Short: "Uninstall all AI skills", + Long: `Remove all installed Databricks AI skills from all coding agents. + +Removes skill directories, symlinks, and the state file.`, + RunE: func(cmd *cobra.Command, args []string) error { + return installer.UninstallSkills(cmd.Context()) + }, + } +} diff --git a/experimental/aitools/cmd/update.go b/experimental/aitools/cmd/update.go new file mode 100644 index 0000000000..510470a34d --- /dev/null +++ b/experimental/aitools/cmd/update.go @@ -0,0 +1,44 @@ +package aitools + +import ( + "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/experimental/aitools/lib/installer" + "github.com/databricks/cli/libs/cmdio" + "github.com/spf13/cobra" +) + +func newUpdateCmd() *cobra.Command { + var check, force, noNew bool + + cmd := &cobra.Command{ + Use: "update", + Short: "Update installed AI skills", + Long: `Update installed Databricks AI skills to the latest release. + +By default, updates all installed skills and auto-installs new skills +from the manifest. Use --no-new to skip new skills, or --check to +preview what would change without downloading.`, + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + installed := agents.DetectInstalled(ctx) + src := &installer.GitHubManifestSource{} + result, err := installer.UpdateSkills(ctx, src, installed, installer.UpdateOptions{ + Check: check, + Force: force, + NoNew: noNew, + }) + if err != nil { + return err + } + if result != nil && (len(result.Updated) > 0 || len(result.Added) > 0) { + cmdio.LogString(ctx, installer.FormatUpdateResult(result)) + } + return nil + }, + } + + cmd.Flags().BoolVar(&check, "check", false, "Show what would be updated without downloading") + cmd.Flags().BoolVar(&force, "force", false, "Re-download even if versions match") + cmd.Flags().BoolVar(&noNew, "no-new", false, "Don't auto-install new skills from manifest") + return cmd +} diff --git a/experimental/aitools/cmd/version.go b/experimental/aitools/cmd/version.go new file mode 100644 index 0000000000..d184dedf58 --- /dev/null +++ b/experimental/aitools/cmd/version.go @@ -0,0 +1,68 @@ +package aitools + +import ( + "fmt" + "strings" + + "github.com/databricks/cli/experimental/aitools/lib/installer" + "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/log" + "github.com/spf13/cobra" +) + +func newVersionCmd() *cobra.Command { + return &cobra.Command{ + Use: "version", + Short: "Show installed AI skills version", + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + + globalDir, err := installer.GlobalSkillsDir(ctx) + if err != nil { + return err + } + + state, err := installer.LoadState(globalDir) + if err != nil { + return fmt.Errorf("failed to load install state: %w", err) + } + + if state == nil { + cmdio.LogString(ctx, "Databricks AI skills: not installed") + cmdio.LogString(ctx, "") + cmdio.LogString(ctx, "Run 'databricks experimental aitools install' to install.") + return nil + } + + version := strings.TrimPrefix(state.Release, "v") + cmdio.LogString(ctx, fmt.Sprintf("Databricks AI skills v%s", version)) + cmdio.LogString(ctx, fmt.Sprintf(" Skills installed: %d", len(state.Skills))) + cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) + + // Best-effort staleness check. + if env.Get(ctx, "DATABRICKS_SKILLS_REF") != "" { + cmdio.LogString(ctx, " Using custom ref: $DATABRICKS_SKILLS_REF") + return nil + } + + src := &installer.GitHubManifestSource{} + latest, err := src.FetchLatestRelease(ctx) + if err != nil { + log.Debugf(ctx, "Could not check for updates: %v", err) + return nil + } + + if latest == state.Release { + cmdio.LogString(ctx, " Status: up to date") + } else { + latestVersion := strings.TrimPrefix(latest, "v") + cmdio.LogString(ctx, fmt.Sprintf(" Status: update available (v%s)", latestVersion)) + cmdio.LogString(ctx, "") + cmdio.LogString(ctx, "Run 'databricks experimental aitools update' to update.") + } + + return nil + }, + } +} diff --git a/experimental/aitools/lib/installer/uninstall.go b/experimental/aitools/lib/installer/uninstall.go new file mode 100644 index 0000000000..e03e749730 --- /dev/null +++ b/experimental/aitools/lib/installer/uninstall.go @@ -0,0 +1,147 @@ +package installer + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/log" +) + +// UninstallSkills removes all installed skills, their symlinks, and the state file. +func UninstallSkills(ctx context.Context) error { + globalDir, err := GlobalSkillsDir(ctx) + if err != nil { + return err + } + + state, err := LoadState(globalDir) + if err != nil { + return fmt.Errorf("failed to load install state: %w", err) + } + + if state == nil { + if hasLegacyInstall(ctx, globalDir) { + return fmt.Errorf("found skills from a previous install without state tracking; run 'databricks experimental aitools install' first, then uninstall") + } + return fmt.Errorf("no skills installed") + } + + skillCount := len(state.Skills) + + // Remove skill directories and symlinks for each skill in state. + for name := range state.Skills { + // Remove canonical skill directory. + canonicalDir := filepath.Join(globalDir, name) + if err := os.RemoveAll(canonicalDir); err != nil { + log.Warnf(ctx, "Failed to remove %s: %v", canonicalDir, err) + } + + // Remove symlinks from ALL agent directories (not just detected ones). + removeSymlinksFromAgents(ctx, name) + } + + // Clean up orphaned symlinks pointing into the canonical dir. + cleanOrphanedSymlinks(ctx, globalDir) + + // Delete state file. + stateFile := filepath.Join(globalDir, stateFileName) + if err := os.Remove(stateFile); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove state file: %w", err) + } + + noun := "skills" + if skillCount == 1 { + noun = "skill" + } + cmdio.LogString(ctx, fmt.Sprintf("Uninstalled %d %s.", skillCount, noun)) + return nil +} + +// removeSymlinksFromAgents removes a skill's symlink from all agent directories in the registry. +func removeSymlinksFromAgents(ctx context.Context, skillName string) { + for i := range agents.Registry { + agent := &agents.Registry[i] + skillsDir, err := agent.SkillsDir(ctx) + if err != nil { + continue + } + + destDir := filepath.Join(skillsDir, skillName) + + // Use Lstat to detect symlinks (Stat follows them). + fi, err := os.Lstat(destDir) + if os.IsNotExist(err) { + continue + } + if err != nil { + log.Warnf(ctx, "Failed to stat %s for %s: %v", destDir, agent.DisplayName, err) + continue + } + + // Remove symlinks and directories alike. + if fi.Mode()&os.ModeSymlink != 0 { + if err := os.Remove(destDir); err != nil { + log.Warnf(ctx, "Failed to remove symlink %s: %v", destDir, err) + } + } else { + if err := os.RemoveAll(destDir); err != nil { + log.Warnf(ctx, "Failed to remove %s: %v", destDir, err) + } + } + + log.Debugf(ctx, "Removed %q from %s", skillName, agent.DisplayName) + } +} + +// cleanOrphanedSymlinks scans all agent skill directories for symlinks pointing +// into globalDir that are not tracked in state, and removes them. +func cleanOrphanedSymlinks(ctx context.Context, globalDir string) { + for i := range agents.Registry { + agent := &agents.Registry[i] + skillsDir, err := agent.SkillsDir(ctx) + if err != nil { + continue + } + + entries, err := os.ReadDir(skillsDir) + if err != nil { + continue + } + + for _, entry := range entries { + entryPath := filepath.Join(skillsDir, entry.Name()) + + fi, err := os.Lstat(entryPath) + if err != nil { + continue + } + + if fi.Mode()&os.ModeSymlink == 0 { + continue + } + + target, err := os.Readlink(entryPath) + if err != nil { + continue + } + + // Check if the symlink points into our global skills dir. + if !strings.HasPrefix(target, globalDir+string(os.PathSeparator)) && target != globalDir { + continue + } + + // This symlink points into our managed dir. Remove it. + if err := os.Remove(entryPath); err != nil { + log.Warnf(ctx, "Failed to remove orphaned symlink %s: %v", entryPath, err) + } else { + log.Debugf(ctx, "Removed orphaned symlink %s -> %s", entryPath, target) + } + } + } +} + diff --git a/experimental/aitools/lib/installer/uninstall_test.go b/experimental/aitools/lib/installer/uninstall_test.go new file mode 100644 index 0000000000..6f0e203209 --- /dev/null +++ b/experimental/aitools/lib/installer/uninstall_test.go @@ -0,0 +1,194 @@ +package installer + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/libs/cmdio" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func installTestSkills(t *testing.T, tmp string) string { + t.Helper() + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + return filepath.Join(tmp, ".databricks", "aitools", "skills") +} + +func TestUninstallRemovesSkillDirectories(t *testing.T) { + tmp := setupTestHome(t) + globalDir := installTestSkills(t, tmp) + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + + err := UninstallSkills(ctx) + require.NoError(t, err) + + // Skill directories should be gone. + _, err = os.Stat(filepath.Join(globalDir, "databricks-sql")) + assert.True(t, os.IsNotExist(err)) + _, err = os.Stat(filepath.Join(globalDir, "databricks-jobs")) + assert.True(t, os.IsNotExist(err)) + + assert.Contains(t, stderr.String(), "Uninstalled 2 skills.") +} + +func TestUninstallRemovesSymlinks(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Use two registry-based agents so uninstall can find them. + // Create config dirs for claude-code and cursor (both in agents.Registry). + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".cursor"), 0o755)) + + claudeAgent := &agents.Agent{ + Name: "claude-code", + DisplayName: "Claude Code", + ConfigDir: func(_ context.Context) (string, error) { + return filepath.Join(tmp, ".claude"), nil + }, + } + cursorAgent := &agents.Agent{ + Name: "cursor", + DisplayName: "Cursor", + ConfigDir: func(_ context.Context) (string, error) { + return filepath.Join(tmp, ".cursor"), nil + }, + } + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{claudeAgent, cursorAgent}, InstallOptions{})) + + ctx2, _ := cmdio.NewTestContextWithStderr(t.Context()) + err := UninstallSkills(ctx2) + require.NoError(t, err) + + // Check that agent skill directories are cleaned up. + // These agents are in agents.Registry so removeSymlinksFromAgents finds them. + for _, agentDir := range []string{".claude", ".cursor"} { + sqlLink := filepath.Join(tmp, agentDir, "skills", "databricks-sql") + _, err := os.Lstat(sqlLink) + assert.True(t, os.IsNotExist(err), "symlink should be removed from %s", agentDir) + + jobsLink := filepath.Join(tmp, agentDir, "skills", "databricks-jobs") + _, err = os.Lstat(jobsLink) + assert.True(t, os.IsNotExist(err), "symlink should be removed from %s", agentDir) + } +} + +func TestUninstallCleansOrphanedSymlinks(t *testing.T) { + tmp := setupTestHome(t) + globalDir := installTestSkills(t, tmp) + + // Create an orphaned symlink in a registry agent's dir that points into + // globalDir but is not tracked in state. + // .claude is in agents.Registry so cleanOrphanedSymlinks will scan it. + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755)) + agentSkillsDir := filepath.Join(tmp, ".claude", "skills") + require.NoError(t, os.MkdirAll(agentSkillsDir, 0o755)) + + orphanTarget := filepath.Join(globalDir, "databricks-orphan") + require.NoError(t, os.MkdirAll(orphanTarget, 0o755)) + orphanLink := filepath.Join(agentSkillsDir, "databricks-orphan") + require.NoError(t, os.Symlink(orphanTarget, orphanLink)) + + ctx, _ := cmdio.NewTestContextWithStderr(t.Context()) + err := UninstallSkills(ctx) + require.NoError(t, err) + + // Orphaned symlink should be removed. + _, err = os.Lstat(orphanLink) + assert.True(t, os.IsNotExist(err)) +} + +func TestUninstallDeletesStateFile(t *testing.T) { + tmp := setupTestHome(t) + globalDir := installTestSkills(t, tmp) + + // Verify state file exists before uninstall. + _, err := os.Stat(filepath.Join(globalDir, ".state.json")) + require.NoError(t, err) + + ctx := cmdio.MockDiscard(t.Context()) + err = UninstallSkills(ctx) + require.NoError(t, err) + + // State file should be gone. + _, err = os.Stat(filepath.Join(globalDir, ".state.json")) + assert.True(t, os.IsNotExist(err)) +} + +func TestUninstallNoStateReturnsError(t *testing.T) { + setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + + err := UninstallSkills(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), "no skills installed") +} + +func TestUninstallHandlesMissingDirectories(t *testing.T) { + tmp := setupTestHome(t) + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + + // Create state manually but without actual skill directories on disk. + state := &InstallState{ + SchemaVersion: 1, + Release: "v0.1.0", + LastUpdated: time.Now(), + Skills: map[string]string{ + "databricks-sql": "0.1.0", + "databricks-jobs": "0.1.0", + }, + } + require.NoError(t, SaveState(globalDir, state)) + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + err := UninstallSkills(ctx) + require.NoError(t, err) + assert.Contains(t, stderr.String(), "Uninstalled 2 skills.") +} + +func TestUninstallHandlesBrokenSymlinks(t *testing.T) { + tmp := setupTestHome(t) + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + + // Create state and a broken symlink. + state := &InstallState{ + SchemaVersion: 1, + Release: "v0.1.0", + LastUpdated: time.Now(), + Skills: map[string]string{ + "databricks-sql": "0.1.0", + }, + } + require.NoError(t, SaveState(globalDir, state)) + + // Create a broken symlink in a registry agent dir (.claude is in agents.Registry). + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755)) + agentSkillsDir := filepath.Join(tmp, ".claude", "skills") + require.NoError(t, os.MkdirAll(agentSkillsDir, 0o755)) + brokenLink := filepath.Join(agentSkillsDir, "databricks-sql") + require.NoError(t, os.Symlink("/nonexistent/target", brokenLink)) + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + err := UninstallSkills(ctx) + require.NoError(t, err) + + // Broken symlink should be removed. + _, err = os.Lstat(brokenLink) + assert.True(t, os.IsNotExist(err)) + assert.Contains(t, stderr.String(), "Uninstalled 1 skill.") +} diff --git a/experimental/aitools/lib/installer/update.go b/experimental/aitools/lib/installer/update.go new file mode 100644 index 0000000000..0488acc3c0 --- /dev/null +++ b/experimental/aitools/lib/installer/update.go @@ -0,0 +1,223 @@ +package installer + +import ( + "context" + "fmt" + "path/filepath" + "sort" + "strings" + "time" + + "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/log" +) + +// UpdateOptions controls the behavior of UpdateSkills. +type UpdateOptions struct { + Force bool + NoNew bool + Check bool // dry run: show what would change without downloading + Skills []string // empty = all installed +} + +// UpdateResult describes what UpdateSkills did (or would do in check mode). +type UpdateResult struct { + Updated []SkillUpdate // skills that were updated + Added []SkillUpdate // new skills added (when NoNew is false) + Unchanged []string // skills at current version + Skipped []string // skills skipped (experimental, version constraint) +} + +// SkillUpdate describes a single skill version change. +type SkillUpdate struct { + Name string + OldVersion string + NewVersion string +} + +// UpdateSkills updates installed skills to the latest release. +func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agents.Agent, opts UpdateOptions) (*UpdateResult, error) { + globalDir, err := GlobalSkillsDir(ctx) + if err != nil { + return nil, err + } + + state, err := LoadState(globalDir) + if err != nil { + return nil, fmt.Errorf("failed to load install state: %w", err) + } + + if state == nil { + if hasLegacyInstall(ctx, globalDir) { + return nil, fmt.Errorf("found skills from a previous install without state tracking; run 'databricks experimental aitools install' to refresh before updating") + } + return nil, fmt.Errorf("no skills installed; run 'databricks experimental aitools install' to install") + } + + latestTag, err := src.FetchLatestRelease(ctx) + if err != nil { + if opts.Check { + log.Warnf(ctx, "Could not check for updates: %v", err) + return &UpdateResult{}, nil + } + return nil, fmt.Errorf("failed to fetch latest release: %w", err) + } + + if state.Release == latestTag && !opts.Force { + cmdio.LogString(ctx, "Already up to date.") + return &UpdateResult{Unchanged: sortedKeys(state.Skills)}, nil + } + + manifest, err := src.FetchManifest(ctx, latestTag) + if err != nil { + if opts.Check { + log.Warnf(ctx, "Could not fetch manifest: %v", err) + return &UpdateResult{}, nil + } + return nil, err + } + + // Determine the skill set to consider. + skillSet := buildUpdateSkillSet(state, manifest, opts) + + result := &UpdateResult{} + + // Sort skill names for deterministic output. + names := sortedKeys(skillSet) + + for _, name := range names { + meta, inManifest := manifest.Skills[name] + oldVersion := state.Skills[name] + + if !inManifest { + // Skill was in state but removed from manifest. Keep as unchanged. + result.Unchanged = append(result.Unchanged, name) + continue + } + + // Check if this is a new skill (not in state). + _, wasInstalled := state.Skills[name] + + if meta.Version == oldVersion && !opts.Force { + result.Unchanged = append(result.Unchanged, name) + continue + } + + update := SkillUpdate{ + Name: name, + OldVersion: oldVersion, + NewVersion: meta.Version, + } + + if !wasInstalled { + result.Added = append(result.Added, update) + } else { + result.Updated = append(result.Updated, update) + } + } + + if opts.Check { + return result, nil + } + + // Download and install updated/added skills. + allChanges := append(result.Updated, result.Added...) + for _, change := range allChanges { + meta := manifest.Skills[change.Name] + if err := installSkillForAgents(ctx, latestTag, change.Name, meta.Files, targetAgents, globalDir); err != nil { + return nil, err + } + } + + // Update state. + state.Release = latestTag + state.LastUpdated = time.Now() + for _, change := range allChanges { + state.Skills[change.Name] = change.NewVersion + } + if err := SaveState(globalDir, state); err != nil { + return nil, err + } + + return result, nil +} + +// buildUpdateSkillSet determines which skills to consider for update. +func buildUpdateSkillSet(state *InstallState, manifest *Manifest, opts UpdateOptions) map[string]bool { + skillSet := make(map[string]bool) + + if len(opts.Skills) > 0 { + // Only named skills. + for _, name := range opts.Skills { + skillSet[name] = true + } + return skillSet + } + + // All installed skills. + for name := range state.Skills { + skillSet[name] = true + } + + // Auto-add new skills from manifest (unless --no-new). + if !opts.NoNew { + for name := range manifest.Skills { + skillSet[name] = true + } + } + + return skillSet +} + +// hasLegacyInstall checks both canonical and legacy dirs for skills on disk without state. +func hasLegacyInstall(ctx context.Context, globalDir string) bool { + if hasSkillsOnDisk(globalDir) { + return true + } + homeDir, err := env.UserHomeDir(ctx) + if err != nil { + return false + } + return hasSkillsOnDisk(filepath.Join(homeDir, ".databricks", "agent-skills")) +} + +// sortedKeys returns the keys of a map sorted alphabetically. +func sortedKeys[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// FormatUpdateResult returns a human-readable summary of the update result. +func FormatUpdateResult(result *UpdateResult) string { + var lines []string + + for _, u := range result.Updated { + if u.OldVersion == "" { + lines = append(lines, fmt.Sprintf(" updated %s -> v%s", u.Name, u.NewVersion)) + } else { + lines = append(lines, fmt.Sprintf(" updated %s v%s -> v%s", u.Name, u.OldVersion, u.NewVersion)) + } + } + + for _, a := range result.Added { + lines = append(lines, fmt.Sprintf(" added %s v%s", a.Name, a.NewVersion)) + } + + total := len(result.Updated) + len(result.Added) + if total == 0 { + return "No changes." + } + + noun := "skills" + if total == 1 { + noun = "skill" + } + lines = append(lines, fmt.Sprintf("Updated %d %s.", total, noun)) + return strings.Join(lines, "\n") +} diff --git a/experimental/aitools/lib/installer/update_test.go b/experimental/aitools/lib/installer/update_test.go new file mode 100644 index 0000000000..c452ec4e42 --- /dev/null +++ b/experimental/aitools/lib/installer/update_test.go @@ -0,0 +1,292 @@ +package installer + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/libs/cmdio" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestUpdateNoStateReturnsInstallHint(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + _ = tmp + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + _, err := UpdateSkills(ctx, src, nil, UpdateOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "no skills installed") + assert.Contains(t, err.Error(), "databricks experimental aitools install") +} + +func TestUpdateLegacyInstallDetected(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + + // Create skills in canonical location but no state file. + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + require.NoError(t, os.MkdirAll(filepath.Join(globalDir, "databricks-sql"), 0o755)) + + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + _, err := UpdateSkills(ctx, src, nil, UpdateOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "previous install without state tracking") + assert.Contains(t, err.Error(), "refresh before updating") +} + +func TestUpdateAlreadyUpToDate(t *testing.T) { + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + + // Install first. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // Reset stderr. + stderr.Reset() + + // Update with same release. + result, err := UpdateSkills(ctx, src, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + assert.Contains(t, stderr.String(), "Already up to date.") + assert.Len(t, result.Unchanged, 2) + assert.Empty(t, result.Updated) + assert.Empty(t, result.Added) +} + +func TestUpdateVersionDiffDetected(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Install v0.1.0. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // Updated manifest with new version for one skill. + updatedManifest := testManifest() + updatedManifest.Skills["databricks-sql"] = SkillMeta{ + Version: "0.2.0", + Files: []string{"SKILL.md"}, + } + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + + result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + + require.Len(t, result.Updated, 1) + assert.Equal(t, "databricks-sql", result.Updated[0].Name) + assert.Equal(t, "0.1.0", result.Updated[0].OldVersion) + assert.Equal(t, "0.2.0", result.Updated[0].NewVersion) + + // databricks-jobs unchanged. + assert.Contains(t, result.Unchanged, "databricks-jobs") + + // State should be updated. + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.Equal(t, "v0.2.0", state.Release) + assert.Equal(t, "0.2.0", state.Skills["databricks-sql"]) +} + +func TestUpdateCheckDryRun(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Install v0.1.0. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // Updated manifest. + updatedManifest := testManifest() + updatedManifest.Skills["databricks-sql"] = SkillMeta{ + Version: "0.2.0", + Files: []string{"SKILL.md"}, + } + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + + // Track fetch calls to verify no downloads happen. + fetchCalls := 0 + orig := fetchFileFn + t.Cleanup(func() { fetchFileFn = orig }) + fetchFileFn = func(_ context.Context, _, _, _ string) ([]byte, error) { + fetchCalls++ + return []byte("content"), nil + } + + result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{Check: true}) + require.NoError(t, err) + + // Should report the diff. + require.Len(t, result.Updated, 1) + assert.Equal(t, "databricks-sql", result.Updated[0].Name) + + // Should NOT have downloaded anything. + assert.Equal(t, 0, fetchCalls) + + // State should be unchanged. + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.Equal(t, "v0.1.0", state.Release) +} + +func TestUpdateForceRedownloads(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Install v0.1.0. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // Track fetch calls on forced update (same release). + fetchCalls := 0 + orig := fetchFileFn + t.Cleanup(func() { fetchFileFn = orig }) + fetchFileFn = func(_ context.Context, _, _, _ string) ([]byte, error) { + fetchCalls++ + return []byte("content"), nil + } + + result, err := UpdateSkills(ctx, src, []*agents.Agent{agent}, UpdateOptions{Force: true}) + require.NoError(t, err) + + // All skills should be in Updated since Force re-downloads everything. + assert.Len(t, result.Updated, 2) + assert.True(t, fetchCalls > 0, "force should trigger downloads") +} + +func TestUpdateAutoAddsNewSkills(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Install v0.1.0 with two skills. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // New manifest with an additional skill. + updatedManifest := testManifest() + updatedManifest.Skills["databricks-notebooks"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + } + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + + result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + + // The new skill should be in Added. + require.Len(t, result.Added, 1) + assert.Equal(t, "databricks-notebooks", result.Added[0].Name) + assert.Equal(t, "0.1.0", result.Added[0].NewVersion) + + // State should include the new skill. + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.Equal(t, "0.1.0", state.Skills["databricks-notebooks"]) +} + +func TestUpdateNoNewIgnoresNewSkills(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Install v0.1.0. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // New manifest with an additional skill. + updatedManifest := testManifest() + updatedManifest.Skills["databricks-notebooks"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + } + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + + result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{NoNew: true}) + require.NoError(t, err) + + // No new skills should be added. + assert.Empty(t, result.Added) + // Existing skills should be unchanged (same version). + assert.Len(t, result.Unchanged, 2) + + // State should NOT include the new skill. + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.NotContains(t, state.Skills, "databricks-notebooks") +} + +func TestUpdateOutputSortedAlphabetically(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Install with skills. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // Update all skills. + updatedManifest := testManifest() + updatedManifest.Skills["databricks-sql"] = SkillMeta{Version: "0.2.0", Files: []string{"SKILL.md"}} + updatedManifest.Skills["databricks-jobs"] = SkillMeta{Version: "0.2.0", Files: []string{"SKILL.md"}} + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + + result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + + require.Len(t, result.Updated, 2) + assert.Equal(t, "databricks-jobs", result.Updated[0].Name) + assert.Equal(t, "databricks-sql", result.Updated[1].Name) +} + +// failingReleaseMock always fails on FetchLatestRelease. +type failingReleaseMock struct { + releaseErr error +} + +func (m *failingReleaseMock) FetchManifest(_ context.Context, _ string) (*Manifest, error) { + return nil, fmt.Errorf("should not be called") +} + +func (m *failingReleaseMock) FetchLatestRelease(_ context.Context) (string, error) { + return "", m.releaseErr +} + +func TestUpdateCheckWithNetworkFailure(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Install first. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // Simulate network failure on release fetch. + failSrc := &failingReleaseMock{releaseErr: fmt.Errorf("network error")} + + result, err := UpdateSkills(ctx, failSrc, []*agents.Agent{agent}, UpdateOptions{Check: true}) + require.NoError(t, err, "check mode should not error on network failure") + assert.NotNil(t, result) +} From c63002f1e65169886bfe9510f7ea426adf6bce14 Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 09:31:27 +0100 Subject: [PATCH 07/12] Fix PR3 review findings: authoritative release, filtering, symlink safety, output format - FetchLatestRelease returns (string, bool, error) so callers can distinguish real API responses from fallback defaults. Update and version commands use the authoritative flag to gate staleness checks. - Update now filters experimental and min_cli_version skills using the same logic as install. Warns when a skill is removed from the manifest. - Uninstall only removes symlinks pointing into the canonical skills dir, preserving user-managed directories and external symlinks. - FormatUpdateResult accepts a check flag to use "Would update" wording. - Version output matches the spec format (Databricks AI Tools header, skill count, staleness status on the same line). - Consistent "no install" messaging across update, uninstall, and version. - Added tests for removed-skill warning, experimental/min_cli_version filtering in update, check-mode output, and symlink-only uninstall. Co-authored-by: Isaac --- experimental/aitools/cmd/update.go | 2 +- experimental/aitools/cmd/version.go | 34 +++- .../aitools/lib/installer/installer.go | 2 +- .../aitools/lib/installer/installer_test.go | 35 ++-- experimental/aitools/lib/installer/source.go | 33 ++-- .../aitools/lib/installer/uninstall.go | 45 +++-- .../aitools/lib/installer/uninstall_test.go | 81 +++++++- experimental/aitools/lib/installer/update.go | 57 ++++-- .../aitools/lib/installer/update_test.go | 181 +++++++++++++++--- 9 files changed, 360 insertions(+), 110 deletions(-) diff --git a/experimental/aitools/cmd/update.go b/experimental/aitools/cmd/update.go index 510470a34d..67dd2916cf 100644 --- a/experimental/aitools/cmd/update.go +++ b/experimental/aitools/cmd/update.go @@ -31,7 +31,7 @@ preview what would change without downloading.`, return err } if result != nil && (len(result.Updated) > 0 || len(result.Added) > 0) { - cmdio.LogString(ctx, installer.FormatUpdateResult(result)) + cmdio.LogString(ctx, installer.FormatUpdateResult(result, check)) } return nil }, diff --git a/experimental/aitools/cmd/version.go b/experimental/aitools/cmd/version.go index d184dedf58..1b752d5f1a 100644 --- a/experimental/aitools/cmd/version.go +++ b/experimental/aitools/cmd/version.go @@ -29,35 +29,51 @@ func newVersionCmd() *cobra.Command { } if state == nil { - cmdio.LogString(ctx, "Databricks AI skills: not installed") + cmdio.LogString(ctx, "No Databricks AI Tools components installed.") cmdio.LogString(ctx, "") - cmdio.LogString(ctx, "Run 'databricks experimental aitools install' to install.") + cmdio.LogString(ctx, "Run 'databricks experimental aitools install' to get started.") return nil } version := strings.TrimPrefix(state.Release, "v") - cmdio.LogString(ctx, fmt.Sprintf("Databricks AI skills v%s", version)) - cmdio.LogString(ctx, fmt.Sprintf(" Skills installed: %d", len(state.Skills))) - cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) + skillNoun := "skills" + if len(state.Skills) == 1 { + skillNoun = "skill" + } // Best-effort staleness check. if env.Get(ctx, "DATABRICKS_SKILLS_REF") != "" { - cmdio.LogString(ctx, " Using custom ref: $DATABRICKS_SKILLS_REF") + cmdio.LogString(ctx, "Databricks AI Tools:") + cmdio.LogString(ctx, fmt.Sprintf(" Skills: v%s (%d %s)", version, len(state.Skills), skillNoun)) + cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) + cmdio.LogString(ctx, " Using custom ref: $DATABRICKS_SKILLS_REF") return nil } src := &installer.GitHubManifestSource{} - latest, err := src.FetchLatestRelease(ctx) + latest, authoritative, err := src.FetchLatestRelease(ctx) if err != nil { log.Debugf(ctx, "Could not check for updates: %v", err) + authoritative = false + } + + cmdio.LogString(ctx, "Databricks AI Tools:") + + if !authoritative { + cmdio.LogString(ctx, fmt.Sprintf(" Skills: v%s (%d %s)", version, len(state.Skills), skillNoun)) + cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) + cmdio.LogString(ctx, " Could not check for latest version.") return nil } if latest == state.Release { - cmdio.LogString(ctx, " Status: up to date") + cmdio.LogString(ctx, fmt.Sprintf(" Skills: v%s (%d %s, up to date)", version, len(state.Skills), skillNoun)) + cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) } else { latestVersion := strings.TrimPrefix(latest, "v") - cmdio.LogString(ctx, fmt.Sprintf(" Status: update available (v%s)", latestVersion)) + cmdio.LogString(ctx, fmt.Sprintf(" Skills: v%s (%d %s)", version, len(state.Skills), skillNoun)) + cmdio.LogString(ctx, fmt.Sprintf(" Update available: v%s", latestVersion)) + cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) cmdio.LogString(ctx, "") cmdio.LogString(ctx, "Run 'databricks experimental aitools update' to update.") } diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index ab1decba2c..13a2c791a9 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -115,7 +115,7 @@ func ListSkills(ctx context.Context) error { // This is the core installation function. Callers are responsible for agent detection, // prompting, and printing the "Installing..." header. func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgents []*agents.Agent, opts InstallOptions) error { - latestTag, err := src.FetchLatestRelease(ctx) + latestTag, _, err := src.FetchLatestRelease(ctx) if err != nil { return fmt.Errorf("failed to fetch latest release: %w", err) } diff --git a/experimental/aitools/lib/installer/installer_test.go b/experimental/aitools/lib/installer/installer_test.go index 7d9ebc95a3..59d2b48fd1 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -18,9 +18,10 @@ import ( // mockManifestSource is a test double for ManifestSource. type mockManifestSource struct { - manifest *Manifest - release string - fetchErr error + manifest *Manifest + release string + authoritative bool + fetchErr error } func (m *mockManifestSource) FetchManifest(_ context.Context, _ string) (*Manifest, error) { @@ -30,8 +31,8 @@ func (m *mockManifestSource) FetchManifest(_ context.Context, _ string) (*Manife return m.manifest, nil } -func (m *mockManifestSource) FetchLatestRelease(_ context.Context) (string, error) { - return m.release, nil +func (m *mockManifestSource) FetchLatestRelease(_ context.Context) (string, bool, error) { + return m.release, m.authoritative, nil } func testManifest() *Manifest { @@ -195,7 +196,7 @@ func TestInstallSkillsForAgentsWritesState(t *testing.T) { ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) @@ -219,7 +220,7 @@ func TestInstallSkillForSingleWritesState(t *testing.T) { ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{ @@ -242,7 +243,7 @@ func TestInstallSkillsSpecificNotFound(t *testing.T) { ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{ @@ -264,7 +265,7 @@ func TestExperimentalSkillsSkippedByDefault(t *testing.T) { Experimental: true, } - src := &mockManifestSource{manifest: manifest, release: "v0.1.0"} + src := &mockManifestSource{manifest: manifest, release: "v0.1.0", authoritative: true} agent := testAgent(tmp) err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) @@ -292,7 +293,7 @@ func TestExperimentalSkillsIncludedWithFlag(t *testing.T) { Experimental: true, } - src := &mockManifestSource{manifest: manifest, release: "v0.1.0"} + src := &mockManifestSource{manifest: manifest, release: "v0.1.0", authoritative: true} agent := testAgent(tmp) err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{ @@ -328,7 +329,7 @@ func TestMinCLIVersionSkipWithWarningForInstallAll(t *testing.T) { MinCLIVer: "0.300.0", } - src := &mockManifestSource{manifest: manifest, release: "v0.1.0"} + src := &mockManifestSource{manifest: manifest, release: "v0.1.0", authoritative: true} agent := testAgent(tmp) err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) @@ -358,7 +359,7 @@ func TestMinCLIVersionHardErrorForInstallSingle(t *testing.T) { MinCLIVer: "0.300.0", } - src := &mockManifestSource{manifest: manifest, release: "v0.1.0"} + src := &mockManifestSource{manifest: manifest, release: "v0.1.0", authoritative: true} agent := testAgent(tmp) err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{ @@ -374,7 +375,7 @@ func TestIdempotentSecondInstallSkips(t *testing.T) { ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) // First install. @@ -403,7 +404,7 @@ func TestIdempotentInstallUpdatesNewVersions(t *testing.T) { ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) // First install. @@ -416,7 +417,7 @@ func TestIdempotentInstallUpdatesNewVersions(t *testing.T) { Version: "0.2.0", Files: []string{"SKILL.md"}, } - src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0", authoritative: true} // Track which skills are fetched. var fetchedSkills []string @@ -452,7 +453,7 @@ func TestLegacyDetectMessagePrinted(t *testing.T) { globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") require.NoError(t, os.MkdirAll(filepath.Join(globalDir, "databricks-sql"), 0o755)) - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) @@ -470,7 +471,7 @@ func TestLegacyDetectLegacyDir(t *testing.T) { legacyDir := filepath.Join(tmp, ".databricks", "agent-skills") require.NoError(t, os.MkdirAll(filepath.Join(legacyDir, "databricks-sql"), 0o755)) - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) diff --git a/experimental/aitools/lib/installer/source.go b/experimental/aitools/lib/installer/source.go index 59039b8b80..c375d108d4 100644 --- a/experimental/aitools/lib/installer/source.go +++ b/experimental/aitools/lib/installer/source.go @@ -16,12 +16,12 @@ type ManifestSource interface { // FetchManifest fetches the skills manifest at the given ref. FetchManifest(ctx context.Context, ref string) (*Manifest, error) - // FetchLatestRelease returns the latest release tag. - // Implementations should fall back to a default ref on network errors rather - // than returning an error. The error return exists for cases where fallback is - // not possible (e.g., mock implementations in tests that want to simulate hard - // failures). - FetchLatestRelease(ctx context.Context) (string, error) + // FetchLatestRelease returns the latest release tag and whether the result + // is authoritative. When authoritative is true, the tag came from a + // successful API call. When false, the tag is a fallback default (e.g., + // due to network failure). Callers should use this to decide whether + // to trust the result for staleness comparisons. + FetchLatestRelease(ctx context.Context) (tag string, authoritative bool, err error) } // GitHubManifestSource fetches manifests and release info from GitHub. @@ -58,15 +58,16 @@ func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (* } // FetchLatestRelease returns the latest release tag from GitHub. -// If DATABRICKS_SKILLS_REF is set, it is returned immediately. -// On any error (network, non-200, parse), falls back to defaultSkillsRepoRef. +// If DATABRICKS_SKILLS_REF is set, it is returned as authoritative. +// On any error (network, non-200, parse), falls back to defaultSkillsRepoRef +// with authoritative=false. // // The DATABRICKS_SKILLS_REF check is intentionally duplicated in getSkillsRef() // because callers may use either the ManifestSource interface directly or the // convenience FetchManifest wrapper. -func (s *GitHubManifestSource) FetchLatestRelease(ctx context.Context) (string, error) { +func (s *GitHubManifestSource) FetchLatestRelease(ctx context.Context) (string, bool, error) { if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" { - return ref, nil + return ref, true, nil } url := fmt.Sprintf("https://api.github.com/repos/%s/%s/releases/latest", @@ -75,20 +76,20 @@ func (s *GitHubManifestSource) FetchLatestRelease(ctx context.Context) (string, req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { log.Debugf(ctx, "Failed to create release request, falling back to %s: %v", defaultSkillsRepoRef, err) - return defaultSkillsRepoRef, nil + return defaultSkillsRepoRef, false, nil } client := &http.Client{Timeout: 15 * time.Second} resp, err := client.Do(req) if err != nil { log.Debugf(ctx, "Failed to fetch latest release, falling back to %s: %v", defaultSkillsRepoRef, err) - return defaultSkillsRepoRef, nil + return defaultSkillsRepoRef, false, nil } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { log.Debugf(ctx, "Latest release returned HTTP %d, falling back to %s", resp.StatusCode, defaultSkillsRepoRef) - return defaultSkillsRepoRef, nil + return defaultSkillsRepoRef, false, nil } var release struct { @@ -96,13 +97,13 @@ func (s *GitHubManifestSource) FetchLatestRelease(ctx context.Context) (string, } if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { log.Debugf(ctx, "Failed to parse release response, falling back to %s: %v", defaultSkillsRepoRef, err) - return defaultSkillsRepoRef, nil + return defaultSkillsRepoRef, false, nil } if release.TagName == "" { log.Debugf(ctx, "Empty tag_name in release response, falling back to %s", defaultSkillsRepoRef) - return defaultSkillsRepoRef, nil + return defaultSkillsRepoRef, false, nil } - return release.TagName, nil + return release.TagName, true, nil } diff --git a/experimental/aitools/lib/installer/uninstall.go b/experimental/aitools/lib/installer/uninstall.go index e03e749730..f472204d52 100644 --- a/experimental/aitools/lib/installer/uninstall.go +++ b/experimental/aitools/lib/installer/uninstall.go @@ -35,14 +35,15 @@ func UninstallSkills(ctx context.Context) error { // Remove skill directories and symlinks for each skill in state. for name := range state.Skills { - // Remove canonical skill directory. canonicalDir := filepath.Join(globalDir, name) + + // Remove symlinks from agent directories (only symlinks pointing to canonical dir). + removeSymlinksFromAgents(ctx, name, canonicalDir) + + // Remove canonical skill directory. if err := os.RemoveAll(canonicalDir); err != nil { log.Warnf(ctx, "Failed to remove %s: %v", canonicalDir, err) } - - // Remove symlinks from ALL agent directories (not just detected ones). - removeSymlinksFromAgents(ctx, name) } // Clean up orphaned symlinks pointing into the canonical dir. @@ -62,8 +63,10 @@ func UninstallSkills(ctx context.Context) error { return nil } -// removeSymlinksFromAgents removes a skill's symlink from all agent directories in the registry. -func removeSymlinksFromAgents(ctx context.Context, skillName string) { +// removeSymlinksFromAgents removes a skill's symlink from all agent directories +// in the registry, but only if the entry is a symlink pointing into canonicalDir. +// Non-symlink directories are left untouched to avoid deleting user-managed content. +func removeSymlinksFromAgents(ctx context.Context, skillName, canonicalDir string) { for i := range agents.Registry { agent := &agents.Registry[i] skillsDir, err := agent.SkillsDir(ctx) @@ -83,18 +86,28 @@ func removeSymlinksFromAgents(ctx context.Context, skillName string) { continue } - // Remove symlinks and directories alike. - if fi.Mode()&os.ModeSymlink != 0 { - if err := os.Remove(destDir); err != nil { - log.Warnf(ctx, "Failed to remove symlink %s: %v", destDir, err) - } - } else { - if err := os.RemoveAll(destDir); err != nil { - log.Warnf(ctx, "Failed to remove %s: %v", destDir, err) - } + if fi.Mode()&os.ModeSymlink == 0 { + log.Debugf(ctx, "Skipping non-symlink %s for %s", destDir, agent.DisplayName) + continue } - log.Debugf(ctx, "Removed %q from %s", skillName, agent.DisplayName) + target, err := os.Readlink(destDir) + if err != nil { + log.Warnf(ctx, "Failed to read symlink %s: %v", destDir, err) + continue + } + + // Only remove if the symlink points into our canonical dir. + if !strings.HasPrefix(target, canonicalDir+string(os.PathSeparator)) && target != canonicalDir { + log.Debugf(ctx, "Skipping symlink %s (points to %s, not %s)", destDir, target, canonicalDir) + continue + } + + if err := os.Remove(destDir); err != nil { + log.Warnf(ctx, "Failed to remove symlink %s: %v", destDir, err) + } else { + log.Debugf(ctx, "Removed %q from %s", skillName, agent.DisplayName) + } } } diff --git a/experimental/aitools/lib/installer/uninstall_test.go b/experimental/aitools/lib/installer/uninstall_test.go index 6f0e203209..8b9e09c360 100644 --- a/experimental/aitools/lib/installer/uninstall_test.go +++ b/experimental/aitools/lib/installer/uninstall_test.go @@ -18,7 +18,7 @@ func installTestSkills(t *testing.T, tmp string) string { ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) @@ -68,7 +68,7 @@ func TestUninstallRemovesSymlinks(t *testing.T) { }, } - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{claudeAgent, cursorAgent}, InstallOptions{})) ctx2, _ := cmdio.NewTestContextWithStderr(t.Context()) @@ -161,11 +161,11 @@ func TestUninstallHandlesMissingDirectories(t *testing.T) { assert.Contains(t, stderr.String(), "Uninstalled 2 skills.") } -func TestUninstallHandlesBrokenSymlinks(t *testing.T) { +func TestUninstallHandlesBrokenSymlinksToCanonicalDir(t *testing.T) { tmp := setupTestHome(t) globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") - // Create state and a broken symlink. + // Create state with one skill. state := &InstallState{ SchemaVersion: 1, Release: "v0.1.0", @@ -176,19 +176,82 @@ func TestUninstallHandlesBrokenSymlinks(t *testing.T) { } require.NoError(t, SaveState(globalDir, state)) - // Create a broken symlink in a registry agent dir (.claude is in agents.Registry). + // Create a symlink pointing to the canonical dir (which doesn't exist on disk). + canonicalTarget := filepath.Join(globalDir, "databricks-sql") require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755)) agentSkillsDir := filepath.Join(tmp, ".claude", "skills") require.NoError(t, os.MkdirAll(agentSkillsDir, 0o755)) - brokenLink := filepath.Join(agentSkillsDir, "databricks-sql") - require.NoError(t, os.Symlink("/nonexistent/target", brokenLink)) + link := filepath.Join(agentSkillsDir, "databricks-sql") + require.NoError(t, os.Symlink(canonicalTarget, link)) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) err := UninstallSkills(ctx) require.NoError(t, err) - // Broken symlink should be removed. - _, err = os.Lstat(brokenLink) + // Symlink pointing to canonical dir should be removed. + _, err = os.Lstat(link) assert.True(t, os.IsNotExist(err)) assert.Contains(t, stderr.String(), "Uninstalled 1 skill.") } + +func TestUninstallLeavesNonCanonicalSymlinks(t *testing.T) { + tmp := setupTestHome(t) + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + + state := &InstallState{ + SchemaVersion: 1, + Release: "v0.1.0", + LastUpdated: time.Now(), + Skills: map[string]string{ + "databricks-sql": "0.1.0", + }, + } + require.NoError(t, SaveState(globalDir, state)) + + // Create a symlink in an agent dir pointing somewhere other than canonical dir. + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755)) + agentSkillsDir := filepath.Join(tmp, ".claude", "skills") + require.NoError(t, os.MkdirAll(agentSkillsDir, 0o755)) + externalLink := filepath.Join(agentSkillsDir, "databricks-sql") + require.NoError(t, os.Symlink("/some/other/place", externalLink)) + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + err := UninstallSkills(ctx) + require.NoError(t, err) + + // Symlink pointing outside canonical dir should be preserved. + _, err = os.Lstat(externalLink) + assert.NoError(t, err, "symlink to external path should not be removed") + assert.Contains(t, stderr.String(), "Uninstalled 1 skill.") +} + +func TestUninstallLeavesNonSymlinkDirectories(t *testing.T) { + tmp := setupTestHome(t) + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + + state := &InstallState{ + SchemaVersion: 1, + Release: "v0.1.0", + LastUpdated: time.Now(), + Skills: map[string]string{ + "databricks-sql": "0.1.0", + }, + } + require.NoError(t, SaveState(globalDir, state)) + + // Create a regular directory (not symlink) in agent skills dir. + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755)) + agentSkillsDir := filepath.Join(tmp, ".claude", "skills") + regularDir := filepath.Join(agentSkillsDir, "databricks-sql") + require.NoError(t, os.MkdirAll(regularDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(regularDir, "custom.md"), []byte("custom"), 0o644)) + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + err := UninstallSkills(ctx) + require.NoError(t, err) + + // Regular directory should be preserved (not a symlink to canonical dir). + _, err = os.Stat(regularDir) + assert.NoError(t, err, "regular directory should not be removed") + assert.Contains(t, stderr.String(), "Uninstalled 1 skill.") +} diff --git a/experimental/aitools/lib/installer/update.go b/experimental/aitools/lib/installer/update.go index 0488acc3c0..2f96dd010f 100644 --- a/experimental/aitools/lib/installer/update.go +++ b/experimental/aitools/lib/installer/update.go @@ -9,9 +9,11 @@ import ( "time" "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" + "golang.org/x/mod/semver" ) // UpdateOptions controls the behavior of UpdateSkills. @@ -53,18 +55,19 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent if hasLegacyInstall(ctx, globalDir) { return nil, fmt.Errorf("found skills from a previous install without state tracking; run 'databricks experimental aitools install' to refresh before updating") } - return nil, fmt.Errorf("no skills installed; run 'databricks experimental aitools install' to install") + return nil, fmt.Errorf("no skills installed. Run 'databricks experimental aitools install' to install") } - latestTag, err := src.FetchLatestRelease(ctx) + latestTag, authoritative, err := src.FetchLatestRelease(ctx) if err != nil { - if opts.Check { - log.Warnf(ctx, "Could not check for updates: %v", err) - return &UpdateResult{}, nil - } return nil, fmt.Errorf("failed to fetch latest release: %w", err) } + if !authoritative && !opts.Force { + cmdio.LogString(ctx, "Could not check for updates (offline?). Use --force to update anyway.") + return &UpdateResult{Unchanged: sortedKeys(state.Skills)}, nil + } + if state.Release == latestTag && !opts.Force { cmdio.LogString(ctx, "Already up to date.") return &UpdateResult{Unchanged: sortedKeys(state.Skills)}, nil @@ -84,6 +87,9 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent result := &UpdateResult{} + cliVersion := build.GetInfo().Version + isDev := strings.HasPrefix(cliVersion, build.DefaultSemver) + // Sort skill names for deterministic output. names := sortedKeys(skillSet) @@ -92,11 +98,28 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent oldVersion := state.Skills[name] if !inManifest { - // Skill was in state but removed from manifest. Keep as unchanged. + _, wasInstalled := state.Skills[name] + if wasInstalled { + log.Warnf(ctx, "Warning: %q not found in manifest %s (keeping installed version).", name, latestTag) + } result.Unchanged = append(result.Unchanged, name) continue } + // Filter experimental skills unless state opted in. + if meta.Experimental && !state.IncludeExperimental { + log.Debugf(ctx, "Skipping experimental skill %s", name) + result.Skipped = append(result.Skipped, name) + continue + } + + // Filter skills requiring a newer CLI version. + if meta.MinCLIVer != "" && !isDev && semver.Compare("v"+cliVersion, "v"+meta.MinCLIVer) < 0 { + log.Warnf(ctx, "Skipping %s: requires CLI version %s (running %s)", name, meta.MinCLIVer, cliVersion) + result.Skipped = append(result.Skipped, name) + continue + } + // Check if this is a new skill (not in state). _, wasInstalled := state.Skills[name] @@ -194,19 +217,29 @@ func sortedKeys[V any](m map[string]V) []string { } // FormatUpdateResult returns a human-readable summary of the update result. -func FormatUpdateResult(result *UpdateResult) string { +// When check is true, output uses "Would update/add" instead of "Updated/Added". +func FormatUpdateResult(result *UpdateResult, check bool) string { var lines []string + updateVerb := "updated" + addVerb := "added" + summaryVerb := "Updated" + if check { + updateVerb = "would update" + addVerb = "would add" + summaryVerb = "Would update" + } + for _, u := range result.Updated { if u.OldVersion == "" { - lines = append(lines, fmt.Sprintf(" updated %s -> v%s", u.Name, u.NewVersion)) + lines = append(lines, fmt.Sprintf(" %s %s -> v%s", updateVerb, u.Name, u.NewVersion)) } else { - lines = append(lines, fmt.Sprintf(" updated %s v%s -> v%s", u.Name, u.OldVersion, u.NewVersion)) + lines = append(lines, fmt.Sprintf(" %s %s v%s -> v%s", updateVerb, u.Name, u.OldVersion, u.NewVersion)) } } for _, a := range result.Added { - lines = append(lines, fmt.Sprintf(" added %s v%s", a.Name, a.NewVersion)) + lines = append(lines, fmt.Sprintf(" %s %s v%s", addVerb, a.Name, a.NewVersion)) } total := len(result.Updated) + len(result.Added) @@ -218,6 +251,6 @@ func FormatUpdateResult(result *UpdateResult) string { if total == 1 { noun = "skill" } - lines = append(lines, fmt.Sprintf("Updated %d %s.", total, noun)) + lines = append(lines, fmt.Sprintf("%s %d %s.", summaryVerb, total, noun)) return strings.Join(lines, "\n") } diff --git a/experimental/aitools/lib/installer/update_test.go b/experimental/aitools/lib/installer/update_test.go index c452ec4e42..420ff3487f 100644 --- a/experimental/aitools/lib/installer/update_test.go +++ b/experimental/aitools/lib/installer/update_test.go @@ -1,14 +1,17 @@ package installer import ( + "bytes" "context" "fmt" + "log/slog" "os" "path/filepath" "testing" "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -18,7 +21,7 @@ func TestUpdateNoStateReturnsInstallHint(t *testing.T) { ctx := cmdio.MockDiscard(t.Context()) _ = tmp - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} _, err := UpdateSkills(ctx, src, nil, UpdateOptions{}) require.Error(t, err) assert.Contains(t, err.Error(), "no skills installed") @@ -33,7 +36,7 @@ func TestUpdateLegacyInstallDetected(t *testing.T) { globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") require.NoError(t, os.MkdirAll(filepath.Join(globalDir, "databricks-sql"), 0o755)) - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} _, err := UpdateSkills(ctx, src, nil, UpdateOptions{}) require.Error(t, err) assert.Contains(t, err.Error(), "previous install without state tracking") @@ -46,7 +49,7 @@ func TestUpdateAlreadyUpToDate(t *testing.T) { setupFetchMock(t) // Install first. - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) @@ -68,7 +71,7 @@ func TestUpdateVersionDiffDetected(t *testing.T) { setupFetchMock(t) // Install v0.1.0. - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) @@ -78,7 +81,7 @@ func TestUpdateVersionDiffDetected(t *testing.T) { Version: "0.2.0", Files: []string{"SKILL.md"}, } - src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0", authoritative: true} result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{}) require.NoError(t, err) @@ -105,7 +108,7 @@ func TestUpdateCheckDryRun(t *testing.T) { setupFetchMock(t) // Install v0.1.0. - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) @@ -115,7 +118,7 @@ func TestUpdateCheckDryRun(t *testing.T) { Version: "0.2.0", Files: []string{"SKILL.md"}, } - src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0", authoritative: true} // Track fetch calls to verify no downloads happen. fetchCalls := 0 @@ -149,7 +152,7 @@ func TestUpdateForceRedownloads(t *testing.T) { setupFetchMock(t) // Install v0.1.0. - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) @@ -176,7 +179,7 @@ func TestUpdateAutoAddsNewSkills(t *testing.T) { setupFetchMock(t) // Install v0.1.0 with two skills. - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) @@ -186,7 +189,7 @@ func TestUpdateAutoAddsNewSkills(t *testing.T) { Version: "0.1.0", Files: []string{"SKILL.md"}, } - src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0", authoritative: true} result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{}) require.NoError(t, err) @@ -209,7 +212,7 @@ func TestUpdateNoNewIgnoresNewSkills(t *testing.T) { setupFetchMock(t) // Install v0.1.0. - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) @@ -219,7 +222,7 @@ func TestUpdateNoNewIgnoresNewSkills(t *testing.T) { Version: "0.1.0", Files: []string{"SKILL.md"}, } - src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0", authoritative: true} result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{NoNew: true}) require.NoError(t, err) @@ -242,7 +245,7 @@ func TestUpdateOutputSortedAlphabetically(t *testing.T) { setupFetchMock(t) // Install with skills. - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) @@ -250,7 +253,7 @@ func TestUpdateOutputSortedAlphabetically(t *testing.T) { updatedManifest := testManifest() updatedManifest.Skills["databricks-sql"] = SkillMeta{Version: "0.2.0", Files: []string{"SKILL.md"}} updatedManifest.Skills["databricks-jobs"] = SkillMeta{Version: "0.2.0", Files: []string{"SKILL.md"}} - src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0"} + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0", authoritative: true} result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{}) require.NoError(t, err) @@ -260,33 +263,153 @@ func TestUpdateOutputSortedAlphabetically(t *testing.T) { assert.Equal(t, "databricks-sql", result.Updated[1].Name) } -// failingReleaseMock always fails on FetchLatestRelease. -type failingReleaseMock struct { - releaseErr error -} +// nonAuthoritativeMock returns a fallback ref with authoritative=false. +type nonAuthoritativeMock struct{} -func (m *failingReleaseMock) FetchManifest(_ context.Context, _ string) (*Manifest, error) { +func (m *nonAuthoritativeMock) FetchManifest(_ context.Context, _ string) (*Manifest, error) { return nil, fmt.Errorf("should not be called") } -func (m *failingReleaseMock) FetchLatestRelease(_ context.Context) (string, error) { - return "", m.releaseErr +func (m *nonAuthoritativeMock) FetchLatestRelease(_ context.Context) (string, bool, error) { + return defaultSkillsRepoRef, false, nil } -func TestUpdateCheckWithNetworkFailure(t *testing.T) { +func TestUpdateNonAuthoritativeWithoutForce(t *testing.T) { tmp := setupTestHome(t) - ctx := cmdio.MockDiscard(t.Context()) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) // Install first. - src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0"} + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} agent := testAgent(tmp) require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) - // Simulate network failure on release fetch. - failSrc := &failingReleaseMock{releaseErr: fmt.Errorf("network error")} + stderr.Reset() + + // Non-authoritative release fetch (offline fallback). + fallbackSrc := &nonAuthoritativeMock{} + result, err := UpdateSkills(ctx, fallbackSrc, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + assert.Contains(t, stderr.String(), "Could not check for updates (offline?)") + assert.Len(t, result.Unchanged, 2) +} + +func TestUpdateSkillRemovedFromManifestWarning(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Capture log output to verify warning. + var logBuf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelWarn})) + ctx = log.NewContext(ctx, logger) + + // Install v0.1.0 with two skills. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // New manifest that removed databricks-jobs. + updatedManifest := &Manifest{ + Version: "1", + UpdatedAt: "2024-02-01", + Skills: map[string]SkillMeta{ + "databricks-sql": {Version: "0.2.0", Files: []string{"SKILL.md"}}, + }, + } + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0", authoritative: true} + + result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + + // databricks-jobs should be kept as unchanged. + assert.Contains(t, result.Unchanged, "databricks-jobs") + // Warning should be logged. + assert.Contains(t, logBuf.String(), "databricks-jobs") + assert.Contains(t, logBuf.String(), "not found in manifest v0.2.0") + + // State should still have databricks-jobs. + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.Contains(t, state.Skills, "databricks-jobs") +} + +func TestUpdateSkipsExperimentalSkills(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + + // Install v0.1.0 (not experimental). + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // New manifest with an experimental skill. + updatedManifest := testManifest() + updatedManifest.Skills["databricks-experimental"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + Experimental: true, + } + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0", authoritative: true} + + result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + + // Experimental skill should be skipped. + assert.Contains(t, result.Skipped, "databricks-experimental") + assert.Empty(t, result.Added) +} + +func TestUpdateSkipsMinCLIVersionSkills(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + setBuildVersion(t, "0.200.0") + + var logBuf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelWarn})) + ctx = log.NewContext(ctx, logger) + + // Install v0.1.0. + src := &mockManifestSource{manifest: testManifest(), release: "v0.1.0", authoritative: true} + agent := testAgent(tmp) + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{})) + + // New manifest with a skill requiring a newer CLI. + updatedManifest := testManifest() + updatedManifest.Skills["databricks-future"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + MinCLIVer: "0.300.0", + } + src2 := &mockManifestSource{manifest: updatedManifest, release: "v0.2.0", authoritative: true} + + result, err := UpdateSkills(ctx, src2, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + + assert.Contains(t, result.Skipped, "databricks-future") + assert.Contains(t, logBuf.String(), "requires CLI version 0.300.0") +} + +func TestFormatUpdateResultCheckMode(t *testing.T) { + result := &UpdateResult{ + Updated: []SkillUpdate{ + {Name: "databricks-sql", OldVersion: "0.1.0", NewVersion: "0.2.0"}, + }, + Added: []SkillUpdate{ + {Name: "databricks-notebooks", NewVersion: "0.1.0"}, + }, + } + + output := FormatUpdateResult(result, false) + assert.Contains(t, output, "updated databricks-sql") + assert.Contains(t, output, "added databricks-notebooks") + assert.Contains(t, output, "Updated 2 skills.") - result, err := UpdateSkills(ctx, failSrc, []*agents.Agent{agent}, UpdateOptions{Check: true}) - require.NoError(t, err, "check mode should not error on network failure") - assert.NotNil(t, result) + checkOutput := FormatUpdateResult(result, true) + assert.Contains(t, checkOutput, "would update databricks-sql") + assert.Contains(t, checkOutput, "would add databricks-notebooks") + assert.Contains(t, checkOutput, "Would update 2 skills.") } From b23b73a589da4c5fbe28003f807ed767c3818fd0 Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 09:32:32 +0100 Subject: [PATCH 08/12] Fix trailing whitespace in uninstall.go --- experimental/aitools/lib/installer/uninstall.go | 1 - 1 file changed, 1 deletion(-) diff --git a/experimental/aitools/lib/installer/uninstall.go b/experimental/aitools/lib/installer/uninstall.go index f472204d52..cd0c723fdb 100644 --- a/experimental/aitools/lib/installer/uninstall.go +++ b/experimental/aitools/lib/installer/uninstall.go @@ -157,4 +157,3 @@ func cleanOrphanedSymlinks(ctx context.Context, globalDir string) { } } } - From e49fcab0760c1316730b3700dfccfdd4f4c13f4e Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 09:38:33 +0100 Subject: [PATCH 09/12] Fix lint issues: perfsprint, testifylint Co-authored-by: Isaac --- experimental/aitools/cmd/version.go | 10 +++++----- experimental/aitools/lib/installer/uninstall.go | 5 +++-- experimental/aitools/lib/installer/update.go | 5 +++-- experimental/aitools/lib/installer/update_test.go | 6 +++--- 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/experimental/aitools/cmd/version.go b/experimental/aitools/cmd/version.go index 1b752d5f1a..4dc91beb3b 100644 --- a/experimental/aitools/cmd/version.go +++ b/experimental/aitools/cmd/version.go @@ -45,7 +45,7 @@ func newVersionCmd() *cobra.Command { if env.Get(ctx, "DATABRICKS_SKILLS_REF") != "" { cmdio.LogString(ctx, "Databricks AI Tools:") cmdio.LogString(ctx, fmt.Sprintf(" Skills: v%s (%d %s)", version, len(state.Skills), skillNoun)) - cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) + cmdio.LogString(ctx, " Last updated: "+state.LastUpdated.Format("2006-01-02")) cmdio.LogString(ctx, " Using custom ref: $DATABRICKS_SKILLS_REF") return nil } @@ -61,19 +61,19 @@ func newVersionCmd() *cobra.Command { if !authoritative { cmdio.LogString(ctx, fmt.Sprintf(" Skills: v%s (%d %s)", version, len(state.Skills), skillNoun)) - cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) + cmdio.LogString(ctx, " Last updated: "+state.LastUpdated.Format("2006-01-02")) cmdio.LogString(ctx, " Could not check for latest version.") return nil } if latest == state.Release { cmdio.LogString(ctx, fmt.Sprintf(" Skills: v%s (%d %s, up to date)", version, len(state.Skills), skillNoun)) - cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) + cmdio.LogString(ctx, " Last updated: "+state.LastUpdated.Format("2006-01-02")) } else { latestVersion := strings.TrimPrefix(latest, "v") cmdio.LogString(ctx, fmt.Sprintf(" Skills: v%s (%d %s)", version, len(state.Skills), skillNoun)) - cmdio.LogString(ctx, fmt.Sprintf(" Update available: v%s", latestVersion)) - cmdio.LogString(ctx, fmt.Sprintf(" Last updated: %s", state.LastUpdated.Format("2006-01-02"))) + cmdio.LogString(ctx, " Update available: v"+latestVersion) + cmdio.LogString(ctx, " Last updated: "+state.LastUpdated.Format("2006-01-02")) cmdio.LogString(ctx, "") cmdio.LogString(ctx, "Run 'databricks experimental aitools update' to update.") } diff --git a/experimental/aitools/lib/installer/uninstall.go b/experimental/aitools/lib/installer/uninstall.go index cd0c723fdb..1253cb8cfb 100644 --- a/experimental/aitools/lib/installer/uninstall.go +++ b/experimental/aitools/lib/installer/uninstall.go @@ -2,6 +2,7 @@ package installer import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -26,9 +27,9 @@ func UninstallSkills(ctx context.Context) error { if state == nil { if hasLegacyInstall(ctx, globalDir) { - return fmt.Errorf("found skills from a previous install without state tracking; run 'databricks experimental aitools install' first, then uninstall") + return errors.New("found skills from a previous install without state tracking; run 'databricks experimental aitools install' first, then uninstall") } - return fmt.Errorf("no skills installed") + return errors.New("no skills installed") } skillCount := len(state.Skills) diff --git a/experimental/aitools/lib/installer/update.go b/experimental/aitools/lib/installer/update.go index 2f96dd010f..6ba461cd10 100644 --- a/experimental/aitools/lib/installer/update.go +++ b/experimental/aitools/lib/installer/update.go @@ -2,6 +2,7 @@ package installer import ( "context" + "errors" "fmt" "path/filepath" "sort" @@ -53,9 +54,9 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent if state == nil { if hasLegacyInstall(ctx, globalDir) { - return nil, fmt.Errorf("found skills from a previous install without state tracking; run 'databricks experimental aitools install' to refresh before updating") + return nil, errors.New("found skills from a previous install without state tracking; run 'databricks experimental aitools install' to refresh before updating") } - return nil, fmt.Errorf("no skills installed. Run 'databricks experimental aitools install' to install") + return nil, errors.New("no skills installed. Run 'databricks experimental aitools install' to install") } latestTag, authoritative, err := src.FetchLatestRelease(ctx) diff --git a/experimental/aitools/lib/installer/update_test.go b/experimental/aitools/lib/installer/update_test.go index 420ff3487f..f5e98ee5eb 100644 --- a/experimental/aitools/lib/installer/update_test.go +++ b/experimental/aitools/lib/installer/update_test.go @@ -3,7 +3,7 @@ package installer import ( "bytes" "context" - "fmt" + "errors" "log/slog" "os" "path/filepath" @@ -170,7 +170,7 @@ func TestUpdateForceRedownloads(t *testing.T) { // All skills should be in Updated since Force re-downloads everything. assert.Len(t, result.Updated, 2) - assert.True(t, fetchCalls > 0, "force should trigger downloads") + assert.Positive(t, fetchCalls, "force should trigger downloads") } func TestUpdateAutoAddsNewSkills(t *testing.T) { @@ -267,7 +267,7 @@ func TestUpdateOutputSortedAlphabetically(t *testing.T) { type nonAuthoritativeMock struct{} func (m *nonAuthoritativeMock) FetchManifest(_ context.Context, _ string) (*Manifest, error) { - return nil, fmt.Errorf("should not be called") + return nil, errors.New("should not be called") } func (m *nonAuthoritativeMock) FetchLatestRelease(_ context.Context) (string, bool, error) { From 8d6f6031cce100cfae75a54f5732711256034e8b Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 09:39:49 +0100 Subject: [PATCH 10/12] Fix gofmt, gofumpt, perfsprint, and staticcheck lint issues --- experimental/aitools/cmd/install_test.go | 2 +- experimental/aitools/cmd/skills.go | 7 ++++--- experimental/aitools/lib/installer/installer_test.go | 7 ++++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/experimental/aitools/cmd/install_test.go b/experimental/aitools/cmd/install_test.go index 8e88f32342..80a19317b1 100644 --- a/experimental/aitools/cmd/install_test.go +++ b/experimental/aitools/cmd/install_test.go @@ -1,8 +1,8 @@ package aitools import ( - "context" "bufio" + "context" "os" "path/filepath" "testing" diff --git a/experimental/aitools/cmd/skills.go b/experimental/aitools/cmd/skills.go index 9550eb6b33..8034f818e3 100644 --- a/experimental/aitools/cmd/skills.go +++ b/experimental/aitools/cmd/skills.go @@ -2,7 +2,8 @@ package aitools import ( "context" - "fmt" + "errors" + "github.com/charmbracelet/huh" "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/experimental/aitools/lib/installer" @@ -13,7 +14,7 @@ import ( // Package-level vars for testability. var ( - promptAgentSelection = defaultPromptAgentSelection + promptAgentSelection = defaultPromptAgentSelection installSkillsForAgentsFn = installer.InstallSkillsForAgents ) @@ -37,7 +38,7 @@ func defaultPromptAgentSelection(ctx context.Context, detected []*agents.Agent) } if len(selected) == 0 { - return nil, fmt.Errorf("at least one agent must be selected") + return nil, errors.New("at least one agent must be selected") } result := make([]*agents.Agent, 0, len(selected)) diff --git a/experimental/aitools/lib/installer/installer_test.go b/experimental/aitools/lib/installer/installer_test.go index 59d2b48fd1..e06910086a 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -481,7 +481,8 @@ func TestLegacyDetectLegacyDir(t *testing.T) { } func TestInstallAllSkillsSignaturePreserved(t *testing.T) { - // Compile-time check that InstallAllSkills satisfies func(context.Context) error. - var fn func(context.Context) error = InstallAllSkills - _ = fn + // Compile-time check that InstallAllSkills matches the expected signature. + // cmd/apps/init.go passes this as func(context.Context) error. + callback := func(fn func(context.Context) error) { _ = fn } + callback(InstallAllSkills) } From edafa6b8314d13a9ff53a2835cdd932f3f4b05dd Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 09:49:33 +0100 Subject: [PATCH 11/12] Add list command, flag-based filtering, and command restructuring Add --skills, --agents, and --include-experimental flags to install command. Add --skills flag to update, uninstall, and version commands. Create new list command with detailed table output showing available/installed skills. Make skills subcommand hidden for backward compat while promoting flat command structure. Add selective uninstall support via UninstallOptions. Co-authored-by: Isaac --- experimental/aitools/cmd/aitools.go | 5 +- experimental/aitools/cmd/install.go | 112 +++++++- experimental/aitools/cmd/install_test.go | 240 +++++++++++++----- experimental/aitools/cmd/list.go | 102 ++++++++ experimental/aitools/cmd/list_test.go | 60 +++++ experimental/aitools/cmd/skills.go | 64 +---- experimental/aitools/cmd/uninstall.go | 21 +- experimental/aitools/cmd/update.go | 14 +- experimental/aitools/cmd/version.go | 10 +- .../aitools/lib/installer/uninstall.go | 61 +++-- .../aitools/lib/installer/uninstall_test.go | 49 ++++ 11 files changed, 598 insertions(+), 140 deletions(-) create mode 100644 experimental/aitools/cmd/list.go create mode 100644 experimental/aitools/cmd/list_test.go diff --git a/experimental/aitools/cmd/aitools.go b/experimental/aitools/cmd/aitools.go index 3ce43a1073..f037ac1a22 100644 --- a/experimental/aitools/cmd/aitools.go +++ b/experimental/aitools/cmd/aitools.go @@ -18,11 +18,12 @@ Provides commands to: } cmd.AddCommand(newInstallCmd()) - cmd.AddCommand(newSkillsCmd()) - cmd.AddCommand(newToolsCmd()) cmd.AddCommand(newUpdateCmd()) cmd.AddCommand(newUninstallCmd()) + cmd.AddCommand(newListCmd()) cmd.AddCommand(newVersionCmd()) + cmd.AddCommand(newSkillsCmd()) + cmd.AddCommand(newToolsCmd()) return cmd } diff --git a/experimental/aitools/cmd/install.go b/experimental/aitools/cmd/install.go index 7bdc86449c..564e70529b 100644 --- a/experimental/aitools/cmd/install.go +++ b/experimental/aitools/cmd/install.go @@ -1,18 +1,118 @@ package aitools import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/databricks/cli/experimental/aitools/lib/agents" + "github.com/databricks/cli/experimental/aitools/lib/installer" + "github.com/databricks/cli/libs/cmdio" "github.com/spf13/cobra" ) func newInstallCmd() *cobra.Command { - return &cobra.Command{ - Use: "install [skill-name]", - Short: "Alias for skills install", - Long: `Alias for "databricks experimental aitools skills install". + var skillsFlag, agentsFlag string + var includeExperimental bool + + cmd := &cobra.Command{ + Use: "install", + Short: "Install AI skills for coding agents", + Long: `Install Databricks AI skills for detected coding agents. + +Skills are installed globally to each agent's skills directory. +When multiple agents are detected, skills are stored in a canonical location +and symlinked to each agent to avoid duplication. -Installs Databricks skills for detected coding agents.`, +Supported agents: Claude Code, Cursor, Codex CLI, OpenCode, GitHub Copilot, Antigravity`, RunE: func(cmd *cobra.Command, args []string) error { - return runSkillsInstall(cmd.Context(), args) + ctx := cmd.Context() + + // Resolve target agents. + var targetAgents []*agents.Agent + if agentsFlag != "" { + var err error + targetAgents, err = resolveAgentNames(ctx, agentsFlag) + if err != nil { + return err + } + } else { + detected := agents.DetectInstalled(ctx) + if len(detected) == 0 { + printNoAgentsMessage(ctx) + return nil + } + + switch { + case len(detected) == 1: + targetAgents = detected + case cmdio.IsPromptSupported(ctx): + var err error + targetAgents, err = promptAgentSelection(ctx, detected) + if err != nil { + return err + } + default: + targetAgents = detected + } + } + + // Build install options. + opts := installer.InstallOptions{ + IncludeExperimental: includeExperimental, + } + if skillsFlag != "" { + opts.SpecificSkills = strings.Split(skillsFlag, ",") + } + + installer.PrintInstallingFor(ctx, targetAgents) + + src := &installer.GitHubManifestSource{} + return installSkillsForAgentsFn(ctx, src, targetAgents, opts) }, } + + cmd.Flags().StringVar(&skillsFlag, "skills", "", "Specific skills to install (comma-separated)") + cmd.Flags().StringVar(&agentsFlag, "agents", "", "Agents to install for (comma-separated, e.g. claude-code,cursor)") + cmd.Flags().BoolVar(&includeExperimental, "include-experimental", false, "Include experimental skills") + return cmd +} + +// resolveAgentNames parses a comma-separated list of agent names and validates +// them against the registry. Returns an error for unrecognized names. +func resolveAgentNames(ctx context.Context, names string) ([]*agents.Agent, error) { + available := make(map[string]*agents.Agent, len(agents.Registry)) + var availableNames []string + for i := range agents.Registry { + a := &agents.Registry[i] + available[a.Name] = a + availableNames = append(availableNames, a.Name) + } + + var result []*agents.Agent + for _, name := range strings.Split(names, ",") { + name = strings.TrimSpace(name) + if name == "" { + continue + } + agent, ok := available[name] + if !ok { + return nil, fmt.Errorf("unknown agent %q. Available agents: %s", name, strings.Join(availableNames, ", ")) + } + result = append(result, agent) + } + + if len(result) == 0 { + return nil, errors.New("no agents specified") + } + return result, nil +} + +// printNoAgentsMessage prints the "no agents detected" message. +func printNoAgentsMessage(ctx context.Context) { + cmdio.LogString(ctx, "No supported coding agents detected.") + cmdio.LogString(ctx, "") + cmdio.LogString(ctx, "Supported agents: Claude Code, Cursor, Codex CLI, OpenCode, GitHub Copilot, Antigravity") + cmdio.LogString(ctx, "Please install at least one coding agent first.") } diff --git a/experimental/aitools/cmd/install_test.go b/experimental/aitools/cmd/install_test.go index 80a19317b1..1209d5315b 100644 --- a/experimental/aitools/cmd/install_test.go +++ b/experimental/aitools/cmd/install_test.go @@ -10,7 +10,6 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/experimental/aitools/lib/installer" "github.com/databricks/cli/libs/cmdio" - "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -47,62 +46,104 @@ func setupTestAgents(t *testing.T) string { return tmp } -func TestInstallCommandsDelegateToSkillsInstall(t *testing.T) { +func TestInstallAllSkillsForAllAgents(t *testing.T) { setupTestAgents(t) calls := setupInstallMock(t) - tests := []struct { - name string - newCmd func() *cobra.Command - args []string - wantAgents int - wantSkills []string - }{ - { - name: "skills install installs all skills for all agents", - newCmd: newSkillsInstallCmd, - wantAgents: 2, - }, - { - name: "skills install forwards skill name", - newCmd: newSkillsInstallCmd, - args: []string{"bundle/review"}, - wantAgents: 2, - wantSkills: []string{"bundle/review"}, - }, - { - name: "top level install installs all skills", - newCmd: newInstallCmd, - wantAgents: 2, - }, - { - name: "top level install forwards skill name", - newCmd: newInstallCmd, - args: []string{"bundle/review"}, - wantAgents: 2, - wantSkills: []string{"bundle/review"}, - }, - } + ctx := cmdio.MockDiscard(t.Context()) + cmd := newInstallCmd() + cmd.SetContext(ctx) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - *calls = nil + err := cmd.RunE(cmd, nil) + require.NoError(t, err) - ctx := cmdio.MockDiscard(t.Context()) - cmd := tt.newCmd() - cmd.SetContext(ctx) + require.Len(t, *calls, 1) + assert.Len(t, (*calls)[0].agents, 2) + assert.Nil(t, (*calls)[0].opts.SpecificSkills) +} - err := cmd.RunE(cmd, tt.args) - require.NoError(t, err) +func TestInstallSpecificSkills(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) - require.Len(t, *calls, 1) - assert.Len(t, (*calls)[0].agents, tt.wantAgents) - assert.Equal(t, tt.wantSkills, (*calls)[0].opts.SpecificSkills) - }) - } + ctx := cmdio.MockDiscard(t.Context()) + cmd := newInstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--skills", "databricks,databricks-apps"}) + + err := cmd.Execute() + require.NoError(t, err) + + require.Len(t, *calls, 1) + assert.Equal(t, []string{"databricks", "databricks-apps"}, (*calls)[0].opts.SpecificSkills) +} + +func TestInstallSingleSkill(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newInstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--skills", "databricks"}) + + err := cmd.Execute() + require.NoError(t, err) + + require.Len(t, *calls, 1) + assert.Equal(t, []string{"databricks"}, (*calls)[0].opts.SpecificSkills) +} + +func TestInstallSpecificAgents(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newInstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--agents", "claude-code"}) + + err := cmd.Execute() + require.NoError(t, err) + + require.Len(t, *calls, 1) + assert.Equal(t, []string{"claude-code"}, (*calls)[0].agents) +} + +func TestInstallUnknownAgentErrors(t *testing.T) { + setupTestAgents(t) + setupInstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newInstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--agents", "invalid-agent"}) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown agent") + assert.Contains(t, err.Error(), "Available agents:") +} + +func TestInstallIncludeExperimental(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newInstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--include-experimental"}) + + err := cmd.Execute() + require.NoError(t, err) + + require.Len(t, *calls, 1) + assert.True(t, (*calls)[0].opts.IncludeExperimental) } -func TestRunSkillsInstallInteractivePrompt(t *testing.T) { +func TestInstallInteractivePrompt(t *testing.T) { setupTestAgents(t) calls := setupInstallMock(t) @@ -112,15 +153,12 @@ func TestRunSkillsInstallInteractivePrompt(t *testing.T) { promptCalled := false promptAgentSelection = func(_ context.Context, detected []*agents.Agent) ([]*agents.Agent, error) { promptCalled = true - // Return only the first agent. return detected[:1], nil } - // Use SetupTest with PromptSupported=true to simulate interactive terminal. ctx, test := cmdio.SetupTest(t.Context(), cmdio.TestOptions{PromptSupported: true}) defer test.Done() - // Drain both pipes in background to prevent blocking. drain := func(r *bufio.Reader) { buf := make([]byte, 4096) for { @@ -133,7 +171,10 @@ func TestRunSkillsInstallInteractivePrompt(t *testing.T) { go drain(test.Stdout) go drain(test.Stderr) - err := runSkillsInstall(ctx, nil) + cmd := newInstallCmd() + cmd.SetContext(ctx) + + err := cmd.RunE(cmd, nil) require.NoError(t, err) assert.True(t, promptCalled, "prompt should be called when 2+ agents detected and interactive") @@ -141,7 +182,7 @@ func TestRunSkillsInstallInteractivePrompt(t *testing.T) { assert.Len(t, (*calls)[0].agents, 1, "only the selected agent should be passed") } -func TestRunSkillsInstallNonInteractiveUsesAllAgents(t *testing.T) { +func TestInstallNonInteractiveUsesAllAgents(t *testing.T) { setupTestAgents(t) calls := setupInstallMock(t) @@ -154,10 +195,11 @@ func TestRunSkillsInstallNonInteractiveUsesAllAgents(t *testing.T) { return detected, nil } - // MockDiscard gives a non-interactive context. ctx := cmdio.MockDiscard(t.Context()) + cmd := newInstallCmd() + cmd.SetContext(ctx) - err := runSkillsInstall(ctx, nil) + err := cmd.RunE(cmd, nil) require.NoError(t, err) assert.False(t, promptCalled, "prompt should not be called in non-interactive mode") @@ -165,15 +207,97 @@ func TestRunSkillsInstallNonInteractiveUsesAllAgents(t *testing.T) { assert.Len(t, (*calls)[0].agents, 2, "all detected agents should be used") } -func TestRunSkillsInstallNoAgents(t *testing.T) { - // Set HOME to empty dir so no agents are detected. +func TestInstallNoAgentsDetected(t *testing.T) { tmp := t.TempDir() t.Setenv("HOME", tmp) calls := setupInstallMock(t) ctx := cmdio.MockDiscard(t.Context()) - err := runSkillsInstall(ctx, nil) + cmd := newInstallCmd() + cmd.SetContext(ctx) + + err := cmd.RunE(cmd, nil) require.NoError(t, err) assert.Empty(t, *calls, "install should not be called when no agents detected") } + +func TestInstallAgentsFlagSkipsPrompt(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + origPrompt := promptAgentSelection + t.Cleanup(func() { promptAgentSelection = origPrompt }) + + promptCalled := false + promptAgentSelection = func(_ context.Context, detected []*agents.Agent) ([]*agents.Agent, error) { + promptCalled = true + return detected, nil + } + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newInstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--agents", "claude-code,cursor"}) + + err := cmd.Execute() + require.NoError(t, err) + + assert.False(t, promptCalled, "prompt should not be called when --agents is specified") + require.Len(t, *calls, 1) + assert.Equal(t, []string{"claude-code", "cursor"}, (*calls)[0].agents) +} + +func TestSkillsInstallDelegatesToInstall(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newSkillsInstallCmd() + cmd.SetContext(ctx) + + err := cmd.RunE(cmd, nil) + require.NoError(t, err) + + require.Len(t, *calls, 1) + assert.Len(t, (*calls)[0].agents, 2) +} + +func TestSkillsInstallForwardsSkillName(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newSkillsInstallCmd() + cmd.SetContext(ctx) + + err := cmd.RunE(cmd, []string{"databricks"}) + require.NoError(t, err) + + require.Len(t, *calls, 1) + assert.Equal(t, []string{"databricks"}, (*calls)[0].opts.SpecificSkills) +} + +func TestResolveAgentNamesValid(t *testing.T) { + ctx := t.Context() + result, err := resolveAgentNames(ctx, "claude-code,cursor") + require.NoError(t, err) + assert.Len(t, result, 2) + assert.Equal(t, "claude-code", result[0].Name) + assert.Equal(t, "cursor", result[1].Name) +} + +func TestResolveAgentNamesUnknown(t *testing.T) { + ctx := t.Context() + _, err := resolveAgentNames(ctx, "claude-code,invalid-agent") + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown agent") + assert.Contains(t, err.Error(), "invalid-agent") +} + +func TestResolveAgentNamesEmpty(t *testing.T) { + ctx := t.Context() + _, err := resolveAgentNames(ctx, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "no agents specified") +} diff --git a/experimental/aitools/cmd/list.go b/experimental/aitools/cmd/list.go new file mode 100644 index 0000000000..24ac1c0828 --- /dev/null +++ b/experimental/aitools/cmd/list.go @@ -0,0 +1,102 @@ +package aitools + +import ( + "fmt" + "sort" + "strings" + "text/tabwriter" + + "github.com/databricks/cli/experimental/aitools/lib/installer" + "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/log" + "github.com/spf13/cobra" +) + +// listSkillsFn is the function used to render the skills list. +// It is a package-level var so tests can replace the data-fetching layer. +var listSkillsFn = defaultListSkills + +func newListCmd() *cobra.Command { + var showSkills bool + + cmd := &cobra.Command{ + Use: "list", + Short: "List installed AI tools components", + RunE: func(cmd *cobra.Command, args []string) error { + // Currently skills is the only component, so always show detailed view. + return listSkillsFn(cmd) + }, + } + + cmd.Flags().BoolVar(&showSkills, "skills", false, "Show detailed skills information") + return cmd +} + +func defaultListSkills(cmd *cobra.Command) error { + ctx := cmd.Context() + + src := &installer.GitHubManifestSource{} + latestTag, _, err := src.FetchLatestRelease(ctx) + if err != nil { + return fmt.Errorf("failed to fetch latest release: %w", err) + } + + manifest, err := src.FetchManifest(ctx, latestTag) + if err != nil { + return fmt.Errorf("failed to fetch manifest: %w", err) + } + + globalDir, err := installer.GlobalSkillsDir(ctx) + if err != nil { + return err + } + + state, err := installer.LoadState(globalDir) + if err != nil { + log.Debugf(ctx, "Could not load install state: %v", err) + } + + // Build sorted list of skill names. + names := make([]string, 0, len(manifest.Skills)) + for name := range manifest.Skills { + names = append(names, name) + } + sort.Strings(names) + + version := strings.TrimPrefix(latestTag, "v") + cmdio.LogString(ctx, "Available skills (v"+version+"):") + cmdio.LogString(ctx, "") + + var buf strings.Builder + tw := tabwriter.NewWriter(&buf, 0, 4, 2, ' ', 0) + fmt.Fprintln(tw, " NAME\tVERSION\tINSTALLED") + + installedCount := 0 + for _, name := range names { + meta := manifest.Skills[name] + + tag := "" + if meta.Experimental { + tag = " [experimental]" + } + + installedStr := "not installed" + if state != nil { + if v, ok := state.Skills[name]; ok { + installedCount++ + if v == meta.Version { + installedStr = "v" + v + " (up to date)" + } else { + installedStr = "v" + v + " (update available)" + } + } + } + + fmt.Fprintf(tw, " %s%s\tv%s\t%s\n", name, tag, meta.Version, installedStr) + } + tw.Flush() + cmdio.LogString(ctx, buf.String()) + + cmdio.LogString(ctx, fmt.Sprintf("%d/%d skills installed (global)", installedCount, len(names))) + return nil +} diff --git a/experimental/aitools/cmd/list_test.go b/experimental/aitools/cmd/list_test.go new file mode 100644 index 0000000000..81a304740d --- /dev/null +++ b/experimental/aitools/cmd/list_test.go @@ -0,0 +1,60 @@ +package aitools + +import ( + "testing" + + "github.com/databricks/cli/libs/cmdio" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestListCommandExists(t *testing.T) { + cmd := newListCmd() + assert.Equal(t, "list", cmd.Use) +} + +func TestListCommandCallsListFn(t *testing.T) { + orig := listSkillsFn + t.Cleanup(func() { listSkillsFn = orig }) + + called := false + listSkillsFn = func(cmd *cobra.Command) error { + called = true + return nil + } + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newListCmd() + cmd.SetContext(ctx) + + err := cmd.RunE(cmd, nil) + require.NoError(t, err) + assert.True(t, called) +} + +func TestListCommandHasSkillsFlag(t *testing.T) { + cmd := newListCmd() + f := cmd.Flags().Lookup("skills") + require.NotNil(t, f, "--skills flag should exist") + assert.Equal(t, "false", f.DefValue) +} + +func TestSkillsListDelegatesToListFn(t *testing.T) { + orig := listSkillsFn + t.Cleanup(func() { listSkillsFn = orig }) + + called := false + listSkillsFn = func(cmd *cobra.Command) error { + called = true + return nil + } + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newSkillsListCmd() + cmd.SetContext(ctx) + + err := cmd.RunE(cmd, nil) + require.NoError(t, err) + assert.True(t, called) +} diff --git a/experimental/aitools/cmd/skills.go b/experimental/aitools/cmd/skills.go index 8034f818e3..f6897b488c 100644 --- a/experimental/aitools/cmd/skills.go +++ b/experimental/aitools/cmd/skills.go @@ -7,8 +7,6 @@ import ( "github.com/charmbracelet/huh" "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/experimental/aitools/lib/installer" - "github.com/databricks/cli/libs/cmdio" - "github.com/fatih/color" "github.com/spf13/cobra" ) @@ -50,11 +48,13 @@ func defaultPromptAgentSelection(ctx context.Context, detected []*agents.Agent) func newSkillsCmd() *cobra.Command { cmd := &cobra.Command{ - Use: "skills", - Short: "Manage Databricks skills for coding agents", - Long: `Manage Databricks skills that extend coding agents with Databricks-specific capabilities.`, + Use: "skills", + Hidden: true, + Short: "Manage Databricks skills for coding agents", + Long: `Manage Databricks skills that extend coding agents with Databricks-specific capabilities.`, } + // Subcommands delegate to the flat top-level commands. cmd.AddCommand(newSkillsListCmd()) cmd.AddCommand(newSkillsInstallCmd()) @@ -66,7 +66,7 @@ func newSkillsListCmd() *cobra.Command { Use: "list", Short: "List available skills", RunE: func(cmd *cobra.Command, args []string) error { - return installer.ListSkills(cmd.Context()) + return listSkillsFn(cmd) }, } } @@ -75,51 +75,15 @@ func newSkillsInstallCmd() *cobra.Command { return &cobra.Command{ Use: "install [skill-name]", Short: "Install Databricks skills for detected coding agents", - Long: `Install Databricks skills to all detected coding agents. - -Skills are installed globally to each agent's skills directory. -When multiple agents are detected, skills are stored in a canonical location -and symlinked to each agent to avoid duplication. - -Supported agents: Claude Code, Cursor, Codex CLI, OpenCode, GitHub Copilot, Antigravity`, RunE: func(cmd *cobra.Command, args []string) error { - return runSkillsInstall(cmd.Context(), args) + // Delegate to the flat install command's logic. + installCmd := newInstallCmd() + installCmd.SetContext(cmd.Context()) + if len(args) > 0 { + // Pass the skill name as a --skills flag. + installCmd.SetArgs([]string{"--skills", args[0]}) + } + return installCmd.Execute() }, } } - -func runSkillsInstall(ctx context.Context, args []string) error { - detected := agents.DetectInstalled(ctx) - if len(detected) == 0 { - cmdio.LogString(ctx, color.YellowString("No supported coding agents detected.")) - cmdio.LogString(ctx, "") - cmdio.LogString(ctx, "Supported agents: Claude Code, Cursor, Codex CLI, OpenCode, GitHub Copilot, Antigravity") - cmdio.LogString(ctx, "Please install at least one coding agent first.") - return nil - } - - var targetAgents []*agents.Agent - switch { - case len(detected) == 1: - targetAgents = detected - case cmdio.IsPromptSupported(ctx): - var err error - targetAgents, err = promptAgentSelection(ctx, detected) - if err != nil { - return err - } - default: - // Non-interactive: install for all detected agents. - targetAgents = detected - } - - installer.PrintInstallingFor(ctx, targetAgents) - - opts := installer.InstallOptions{} - if len(args) > 0 { - opts.SpecificSkills = []string{args[0]} - } - - src := &installer.GitHubManifestSource{} - return installSkillsForAgentsFn(ctx, src, targetAgents, opts) -} diff --git a/experimental/aitools/cmd/uninstall.go b/experimental/aitools/cmd/uninstall.go index 9edd360100..663d305506 100644 --- a/experimental/aitools/cmd/uninstall.go +++ b/experimental/aitools/cmd/uninstall.go @@ -1,19 +1,30 @@ package aitools import ( + "strings" + "github.com/databricks/cli/experimental/aitools/lib/installer" "github.com/spf13/cobra" ) func newUninstallCmd() *cobra.Command { - return &cobra.Command{ + var skillsFlag string + + cmd := &cobra.Command{ Use: "uninstall", - Short: "Uninstall all AI skills", - Long: `Remove all installed Databricks AI skills from all coding agents. + Short: "Uninstall AI skills", + Long: `Remove installed Databricks AI skills from all coding agents. -Removes skill directories, symlinks, and the state file.`, +By default, removes all skills. Use --skills to remove specific skills only.`, RunE: func(cmd *cobra.Command, args []string) error { - return installer.UninstallSkills(cmd.Context()) + opts := installer.UninstallOptions{} + if skillsFlag != "" { + opts.Skills = strings.Split(skillsFlag, ",") + } + return installer.UninstallSkillsOpts(cmd.Context(), opts) }, } + + cmd.Flags().StringVar(&skillsFlag, "skills", "", "Specific skills to uninstall (comma-separated)") + return cmd } diff --git a/experimental/aitools/cmd/update.go b/experimental/aitools/cmd/update.go index 67dd2916cf..46d2d92b4c 100644 --- a/experimental/aitools/cmd/update.go +++ b/experimental/aitools/cmd/update.go @@ -1,6 +1,8 @@ package aitools import ( + "strings" + "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/experimental/aitools/lib/installer" "github.com/databricks/cli/libs/cmdio" @@ -9,6 +11,7 @@ import ( func newUpdateCmd() *cobra.Command { var check, force, noNew bool + var skillsFlag string cmd := &cobra.Command{ Use: "update", @@ -22,11 +25,17 @@ preview what would change without downloading.`, ctx := cmd.Context() installed := agents.DetectInstalled(ctx) src := &installer.GitHubManifestSource{} - result, err := installer.UpdateSkills(ctx, src, installed, installer.UpdateOptions{ + + opts := installer.UpdateOptions{ Check: check, Force: force, NoNew: noNew, - }) + } + if skillsFlag != "" { + opts.Skills = strings.Split(skillsFlag, ",") + } + + result, err := installer.UpdateSkills(ctx, src, installed, opts) if err != nil { return err } @@ -40,5 +49,6 @@ preview what would change without downloading.`, cmd.Flags().BoolVar(&check, "check", false, "Show what would be updated without downloading") cmd.Flags().BoolVar(&force, "force", false, "Re-download even if versions match") cmd.Flags().BoolVar(&noNew, "no-new", false, "Don't auto-install new skills from manifest") + cmd.Flags().StringVar(&skillsFlag, "skills", "", "Specific skills to update (comma-separated)") return cmd } diff --git a/experimental/aitools/cmd/version.go b/experimental/aitools/cmd/version.go index 4dc91beb3b..f57f62f8a8 100644 --- a/experimental/aitools/cmd/version.go +++ b/experimental/aitools/cmd/version.go @@ -12,10 +12,15 @@ import ( ) func newVersionCmd() *cobra.Command { - return &cobra.Command{ + var showSkills bool + + cmd := &cobra.Command{ Use: "version", Short: "Show installed AI skills version", RunE: func(cmd *cobra.Command, args []string) error { + // showSkills is accepted for forward-compat but currently + // skills is the only component, so the output is the same. + _ = showSkills ctx := cmd.Context() globalDir, err := installer.GlobalSkillsDir(ctx) @@ -81,4 +86,7 @@ func newVersionCmd() *cobra.Command { return nil }, } + + cmd.Flags().BoolVar(&showSkills, "skills", false, "Show detailed skills version information") + return cmd } diff --git a/experimental/aitools/lib/installer/uninstall.go b/experimental/aitools/lib/installer/uninstall.go index 1253cb8cfb..e0d9a63083 100644 --- a/experimental/aitools/lib/installer/uninstall.go +++ b/experimental/aitools/lib/installer/uninstall.go @@ -13,8 +13,20 @@ import ( "github.com/databricks/cli/libs/log" ) +// UninstallOptions controls the behavior of UninstallSkillsOpts. +type UninstallOptions struct { + Skills []string // empty = all +} + // UninstallSkills removes all installed skills, their symlinks, and the state file. func UninstallSkills(ctx context.Context) error { + return UninstallSkillsOpts(ctx, UninstallOptions{}) +} + +// UninstallSkillsOpts removes installed skills based on options. +// When opts.Skills is empty, all skills are removed (same as UninstallSkills). +// When opts.Skills is non-empty, only the named skills are removed. +func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { globalDir, err := GlobalSkillsDir(ctx) if err != nil { return err @@ -32,35 +44,52 @@ func UninstallSkills(ctx context.Context) error { return errors.New("no skills installed") } - skillCount := len(state.Skills) + // Determine which skills to remove. + var toRemove []string + if len(opts.Skills) > 0 { + for _, name := range opts.Skills { + if _, ok := state.Skills[name]; !ok { + return fmt.Errorf("skill %q is not installed", name) + } + toRemove = append(toRemove, name) + } + } else { + for name := range state.Skills { + toRemove = append(toRemove, name) + } + } + + removeAll := len(toRemove) == len(state.Skills) - // Remove skill directories and symlinks for each skill in state. - for name := range state.Skills { + // Remove skill directories and symlinks for each skill. + for _, name := range toRemove { canonicalDir := filepath.Join(globalDir, name) - - // Remove symlinks from agent directories (only symlinks pointing to canonical dir). removeSymlinksFromAgents(ctx, name, canonicalDir) - - // Remove canonical skill directory. if err := os.RemoveAll(canonicalDir); err != nil { log.Warnf(ctx, "Failed to remove %s: %v", canonicalDir, err) } + delete(state.Skills, name) } - // Clean up orphaned symlinks pointing into the canonical dir. - cleanOrphanedSymlinks(ctx, globalDir) - - // Delete state file. - stateFile := filepath.Join(globalDir, stateFileName) - if err := os.Remove(stateFile); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to remove state file: %w", err) + if removeAll { + // Clean up orphaned symlinks and delete state file. + cleanOrphanedSymlinks(ctx, globalDir) + stateFile := filepath.Join(globalDir, stateFileName) + if err := os.Remove(stateFile); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove state file: %w", err) + } + } else { + // Update state to reflect remaining skills. + if err := SaveState(globalDir, state); err != nil { + return err + } } noun := "skills" - if skillCount == 1 { + if len(toRemove) == 1 { noun = "skill" } - cmdio.LogString(ctx, fmt.Sprintf("Uninstalled %d %s.", skillCount, noun)) + cmdio.LogString(ctx, fmt.Sprintf("Uninstalled %d %s.", len(toRemove), noun)) return nil } diff --git a/experimental/aitools/lib/installer/uninstall_test.go b/experimental/aitools/lib/installer/uninstall_test.go index 8b9e09c360..fe52daf37a 100644 --- a/experimental/aitools/lib/installer/uninstall_test.go +++ b/experimental/aitools/lib/installer/uninstall_test.go @@ -255,3 +255,52 @@ func TestUninstallLeavesNonSymlinkDirectories(t *testing.T) { assert.NoError(t, err, "regular directory should not be removed") assert.Contains(t, stderr.String(), "Uninstalled 1 skill.") } + +func TestUninstallSelectiveRemovesOnlyNamedSkills(t *testing.T) { + tmp := setupTestHome(t) + globalDir := installTestSkills(t, tmp) + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + err := UninstallSkillsOpts(ctx, UninstallOptions{Skills: []string{"databricks-sql"}}) + require.NoError(t, err) + + // databricks-sql should be gone. + _, err = os.Stat(filepath.Join(globalDir, "databricks-sql")) + assert.True(t, os.IsNotExist(err)) + + // databricks-jobs should still exist. + _, err = os.Stat(filepath.Join(globalDir, "databricks-jobs")) + assert.NoError(t, err) + + // State should still exist with remaining skill. + state, err := LoadState(globalDir) + require.NoError(t, err) + require.NotNil(t, state) + assert.Len(t, state.Skills, 1) + assert.Contains(t, state.Skills, "databricks-jobs") + + assert.Contains(t, stderr.String(), "Uninstalled 1 skill.") +} + +func TestUninstallSelectiveUnknownSkillErrors(t *testing.T) { + tmp := setupTestHome(t) + installTestSkills(t, tmp) + + ctx := cmdio.MockDiscard(t.Context()) + err := UninstallSkillsOpts(ctx, UninstallOptions{Skills: []string{"nonexistent"}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "not installed") +} + +func TestUninstallSelectiveAllRemovesStateFile(t *testing.T) { + tmp := setupTestHome(t) + globalDir := installTestSkills(t, tmp) + + ctx, _ := cmdio.NewTestContextWithStderr(t.Context()) + err := UninstallSkillsOpts(ctx, UninstallOptions{Skills: []string{"databricks-sql", "databricks-jobs"}}) + require.NoError(t, err) + + // State file should be gone since all skills were removed. + _, err = os.Stat(filepath.Join(globalDir, ".state.json")) + assert.True(t, os.IsNotExist(err)) +} From f2fba9a6a06bf1ad618a70102e80f9600de364ee Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 09:58:24 +0100 Subject: [PATCH 12/12] Fix PR4 review findings: skills install compat, dead code, yellow warning - Always call SetArgs in skills install backward-compat alias (fixes Execute path inheriting parent args) - Add cobra.MaximumNArgs(1) to reject extra positional args - Remove dead installer.ListSkills function (replaced by list command) - Restore yellow color for "No agents detected" warning in install.go - Add Execute-path tests for skills install alias Co-authored-by: Isaac --- experimental/aitools/cmd/install.go | 3 +- experimental/aitools/cmd/install_test.go | 46 +++++++++++++++++++ experimental/aitools/cmd/skills.go | 3 ++ .../aitools/lib/installer/installer.go | 20 -------- 4 files changed, 51 insertions(+), 21 deletions(-) diff --git a/experimental/aitools/cmd/install.go b/experimental/aitools/cmd/install.go index 564e70529b..4ab020fd88 100644 --- a/experimental/aitools/cmd/install.go +++ b/experimental/aitools/cmd/install.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/experimental/aitools/lib/installer" "github.com/databricks/cli/libs/cmdio" + "github.com/fatih/color" "github.com/spf13/cobra" ) @@ -111,7 +112,7 @@ func resolveAgentNames(ctx context.Context, names string) ([]*agents.Agent, erro // printNoAgentsMessage prints the "no agents detected" message. func printNoAgentsMessage(ctx context.Context) { - cmdio.LogString(ctx, "No supported coding agents detected.") + cmdio.LogString(ctx, color.YellowString("No supported coding agents detected.")) cmdio.LogString(ctx, "") cmdio.LogString(ctx, "Supported agents: Claude Code, Cursor, Codex CLI, OpenCode, GitHub Copilot, Antigravity") cmdio.LogString(ctx, "Please install at least one coding agent first.") diff --git a/experimental/aitools/cmd/install_test.go b/experimental/aitools/cmd/install_test.go index 1209d5315b..d83fb414db 100644 --- a/experimental/aitools/cmd/install_test.go +++ b/experimental/aitools/cmd/install_test.go @@ -278,6 +278,52 @@ func TestSkillsInstallForwardsSkillName(t *testing.T) { assert.Equal(t, []string{"databricks"}, (*calls)[0].opts.SpecificSkills) } +func TestSkillsInstallExecuteNoArgs(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newSkillsInstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{}) + + err := cmd.Execute() + require.NoError(t, err) + + require.Len(t, *calls, 1) + assert.Len(t, (*calls)[0].agents, 2) + assert.Nil(t, (*calls)[0].opts.SpecificSkills) +} + +func TestSkillsInstallExecuteWithSkillName(t *testing.T) { + setupTestAgents(t) + calls := setupInstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := newSkillsInstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"databricks"}) + + err := cmd.Execute() + require.NoError(t, err) + + require.Len(t, *calls, 1) + assert.Equal(t, []string{"databricks"}, (*calls)[0].opts.SpecificSkills) +} + +func TestSkillsInstallExecuteRejectsTwoArgs(t *testing.T) { + ctx := cmdio.MockDiscard(t.Context()) + cmd := newSkillsInstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"a", "b"}) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "accepts at most 1 arg") +} + func TestResolveAgentNamesValid(t *testing.T) { ctx := t.Context() result, err := resolveAgentNames(ctx, "claude-code,cursor") diff --git a/experimental/aitools/cmd/skills.go b/experimental/aitools/cmd/skills.go index f6897b488c..2b21e0e703 100644 --- a/experimental/aitools/cmd/skills.go +++ b/experimental/aitools/cmd/skills.go @@ -75,6 +75,7 @@ func newSkillsInstallCmd() *cobra.Command { return &cobra.Command{ Use: "install [skill-name]", Short: "Install Databricks skills for detected coding agents", + Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { // Delegate to the flat install command's logic. installCmd := newInstallCmd() @@ -82,6 +83,8 @@ func newSkillsInstallCmd() *cobra.Command { if len(args) > 0 { // Pass the skill name as a --skills flag. installCmd.SetArgs([]string{"--skills", args[0]}) + } else { + installCmd.SetArgs([]string{}) } return installCmd.Execute() }, diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 13a2c791a9..146a68ab5d 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -91,26 +91,6 @@ func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byt return io.ReadAll(resp.Body) } -// ListSkills fetches and prints available skills. -func ListSkills(ctx context.Context) error { - manifest, err := FetchManifest(ctx) - if err != nil { - return err - } - - cmdio.LogString(ctx, "Available skills:") - cmdio.LogString(ctx, "") - - for name, meta := range manifest.Skills { - cmdio.LogString(ctx, fmt.Sprintf(" %s (v%s)", name, meta.Version)) - } - - cmdio.LogString(ctx, "") - cmdio.LogString(ctx, "Install all with: databricks experimental aitools skills install") - cmdio.LogString(ctx, "Install one with: databricks experimental aitools skills install ") - return nil -} - // InstallSkillsForAgents fetches the manifest and installs skills for the given agents. // This is the core installation function. Callers are responsible for agent detection, // prompting, and printing the "Installing..." header.