diff --git a/experimental/aitools/cmd/install_test.go b/experimental/aitools/cmd/install_test.go index 64f97ddf9c..80a19317b1 100644 --- a/experimental/aitools/cmd/install_test.go +++ b/experimental/aitools/cmd/install_test.go @@ -1,75 +1,179 @@ package aitools import ( + "bufio" "context" + "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..8034f818e3 100644 --- a/experimental/aitools/cmd/skills.go +++ b/experimental/aitools/cmd/skills.go @@ -2,16 +2,52 @@ package aitools import ( "context" + "errors" + "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, errors.New("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 +89,37 @@ 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..2af50222ea 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -7,12 +7,17 @@ import ( "net/http" "os" "path/filepath" + "sort" + "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 +27,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 +55,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 +69,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 +112,184 @@ 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 in sorted order for determinism. + skillNames := make([]string, 0, len(targetSkills)) + for name := range targetSkills { + skillNames = append(skillNames, name) + } + sort.Strings(skillNames) + + for _, name := range skillNames { + meta := targetSkills[name] + // 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. Merge into existing state so skills from previous installs + // (e.g., experimental skills from a prior run) are preserved. + existingState, loadErr := LoadState(globalDir) + if loadErr != nil { + log.Warnf(ctx, "Could not reload state for merge: %v", loadErr) + } + 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") + noun := "skills" + if len(targetSkills) == 1 { + noun = "skill" + } + cmdio.LogString(ctx, fmt.Sprintf("Installed %d %s (v%s).", len(targetSkills), noun, 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 --include-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 +299,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 +401,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 +415,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..791ba6ffbd 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -1,15 +1,93 @@ package installer import ( + "bytes" + "context" + "log/slog" "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/databricks/cli/libs/log" "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 +187,300 @@ 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 skill (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") + + // 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", + 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).") + assert.Contains(t, logBuf.String(), "requires CLI version 0.300.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. + callback := func(fn func(context.Context) error) { _ = fn } + callback(InstallAllSkills) +} 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)