From e5d060884fd9f3fdc4d6ed537713bda8ed66ea60 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 30 Jun 2026 11:52:29 +0200 Subject: [PATCH 1/3] Harden aitools raw skill installs --- .../install-experimental-empty/output.txt | 5 +- .../skills/install-specific/output.txt | 7 + .../aitools/skills/install/output.txt | 7 + .../aitools/skills/update-prune/output.txt | 7 + cmd/aitools/install.go | 1 + cmd/aitools/install_test.go | 3 +- internal/build/cli-compat.json | 2 +- libs/aitools/installer/dump.go | 3 +- libs/aitools/installer/installer.go | 235 +++++++++++++++--- libs/aitools/installer/installer_test.go | 131 +++++++++- .../installer/installer_windows_test.go | 59 +++++ libs/clicompat/clicompat.go | 2 +- 12 files changed, 421 insertions(+), 41 deletions(-) create mode 100644 libs/aitools/installer/installer_windows_test.go diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt b/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt index 188ea02fa3..71fa9e5604 100644 --- a/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt @@ -5,5 +5,8 @@ Command "install" is deprecated, use "databricks aitools install" instead. Flag --global has been deprecated, use --scope=global Installing Databricks skills for Claude Code... Using skills version test-ref -Warn: --experimental was set but the manifest at test-ref exposes no experimental skills. Set DATABRICKS_SKILLS_REF to a release that includes them (or =main for the latest). +Fetching skills manifest... +Warn: --experimental was set but the manifest at test-ref exposes no experimental skills. Set DATABRICKS_SKILLS_REF to a release or ref that includes them. +Downloading test-stable... +Exposing test-stable to 1 agent... Installed 1 skill. diff --git a/acceptance/experimental/aitools/skills/install-specific/output.txt b/acceptance/experimental/aitools/skills/install-specific/output.txt index 74e415457d..3200ae3727 100644 --- a/acceptance/experimental/aitools/skills/install-specific/output.txt +++ b/acceptance/experimental/aitools/skills/install-specific/output.txt @@ -5,6 +5,9 @@ Command "install" is deprecated, use "databricks aitools install" instead. Flag --global has been deprecated, use --scope=global Installing Databricks skills for Claude Code... Using skills version test-ref +Fetching skills manifest... +Downloading test-stable-a... +Exposing test-stable-a to 1 agent... Installed 1 skill. === install a specific experimental skill @@ -13,6 +16,9 @@ Command "install" is deprecated, use "databricks aitools install" instead. Flag --global has been deprecated, use --scope=global Installing Databricks skills for Claude Code... Using skills version test-ref +Fetching skills manifest... +Downloading test-exp... +Exposing test-exp to 1 agent... Installed 1 skill. === asking for an experimental skill without --experimental flag errors out @@ -21,6 +27,7 @@ Command "install" is deprecated, use "databricks aitools install" instead. Flag --global has been deprecated, use --scope=global Installing Databricks skills for Claude Code... Using skills version test-ref +Fetching skills manifest... Error: skill "test-exp" is experimental; use --experimental to install Exit code: 1 diff --git a/acceptance/experimental/aitools/skills/install/output.txt b/acceptance/experimental/aitools/skills/install/output.txt index 2cc5980302..77cd855159 100644 --- a/acceptance/experimental/aitools/skills/install/output.txt +++ b/acceptance/experimental/aitools/skills/install/output.txt @@ -5,6 +5,9 @@ Command "install" is deprecated, use "databricks aitools install" instead. Flag --global has been deprecated, use --scope=global Installing Databricks skills for Claude Code... Using skills version test-ref +Fetching skills manifest... +Downloading test-stable... +Exposing test-stable to 1 agent... Installed 1 skill. === re-run with --experimental installs the experimental one too @@ -13,6 +16,9 @@ Command "install" is deprecated, use "databricks aitools install" instead. Flag --global has been deprecated, use --scope=global Installing Databricks skills for Claude Code... Using skills version test-ref +Fetching skills manifest... +Downloading test-exp... +Exposing test-exp to 1 agent... Installed 2 skills. === no-op re-run is idempotent (no new fetches, no errors) @@ -21,4 +27,5 @@ Command "install" is deprecated, use "databricks aitools install" instead. Flag --global has been deprecated, use --scope=global Installing Databricks skills for Claude Code... Using skills version test-ref +Fetching skills manifest... Installed 2 skills. diff --git a/acceptance/experimental/aitools/skills/update-prune/output.txt b/acceptance/experimental/aitools/skills/update-prune/output.txt index 8664f1fce3..b6d9c3ed25 100644 --- a/acceptance/experimental/aitools/skills/update-prune/output.txt +++ b/acceptance/experimental/aitools/skills/update-prune/output.txt @@ -5,11 +5,18 @@ Command "install" is deprecated, use "databricks aitools install" instead. Flag --global has been deprecated, use --scope=global Installing Databricks skills for Claude Code... Using skills version test-ref +Fetching skills manifest... +Downloading alpha... +Exposing alpha to 1 agent... +Downloading beta... +Exposing beta to 1 agent... Installed 2 skills. === update against a release where beta is gone: alpha updates, beta is pruned >>> DATABRICKS_SKILLS_REF=v2-ref [CLI] experimental aitools update --scope global Command "update" is deprecated, use "databricks aitools update" instead. +Downloading alpha... +Exposing alpha to 1 agent... updated alpha v1.0.0 -> v2.0.0 removed beta (no longer in this release) Updated 1 skill. diff --git a/cmd/aitools/install.go b/cmd/aitools/install.go index bfd78cc28e..eca459e14e 100644 --- a/cmd/aitools/install.go +++ b/cmd/aitools/install.go @@ -368,6 +368,7 @@ func executePlan(ctx context.Context, src installer.ManifestSource, plan []agent } records := map[string]installer.PluginRecord{} for _, it := range pluginItems { + cmdio.LogString(ctx, fmt.Sprintf("Installing databricks plugin for %s...", it.agent.DisplayName)) rec, err := installPluginForAgentFn(ctx, it.agent, it.scope, ref) if err != nil { cmdio.LogString(ctx, cmdio.Yellow(ctx, fmt.Sprintf("Skipped %s: %v", it.agent.DisplayName, err))) diff --git a/cmd/aitools/install_test.go b/cmd/aitools/install_test.go index 008186eef0..415821829e 100644 --- a/cmd/aitools/install_test.go +++ b/cmd/aitools/install_test.go @@ -286,7 +286,7 @@ func TestInstallPluginFirstDefault(t *testing.T) { plugins := setupPluginMock(t) skills := setupInstallMock(t) - ctx := cmdio.MockDiscard(t.Context()) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) cmd := NewInstallCmd() cmd.SetContext(ctx) @@ -298,6 +298,7 @@ func TestInstallPluginFirstDefault(t *testing.T) { // plugin) does, plus a plugin recommendation. require.Len(t, *skills, 1) assert.Equal(t, []string{agents.NameCursor}, (*skills)[0].agents) + assert.Contains(t, stderr.String(), "Installing databricks plugin for Claude Code...") } func TestInstallInteractivePickerAndConfirm(t *testing.T) { diff --git a/internal/build/cli-compat.json b/internal/build/cli-compat.json index d5645e2501..fc1855cf35 100644 --- a/internal/build/cli-compat.json +++ b/internal/build/cli-compat.json @@ -1,4 +1,4 @@ { - "1.0.0": { "appkit": "0.38.1", "skills": "latest" }, + "1.0.0": { "appkit": "0.38.1", "skills": "0.2.9" }, "0.299.2": { "appkit": "0.24.0", "skills": "0.1.5" } } diff --git a/libs/aitools/installer/dump.go b/libs/aitools/installer/dump.go index 408a09262f..e9a1083b93 100644 --- a/libs/aitools/installer/dump.go +++ b/libs/aitools/installer/dump.go @@ -6,7 +6,6 @@ import ( "maps" "path/filepath" "slices" - "strings" "github.com/databricks/cli/libs/cmdio" ) @@ -21,7 +20,7 @@ func DumpSkillsToPath(ctx context.Context, src ManifestSource, destDir string, o if err != nil { return 0, err } - cmdio.LogString(ctx, "Using skills version "+strings.TrimPrefix(ref, "v")) + cmdio.LogString(ctx, "Using skills version "+DisplaySkillsVersion(ref)) manifest, ref, err := FetchSkillsManifestWithFallback(ctx, src, ref, !explicit) if err != nil { diff --git a/libs/aitools/installer/installer.go b/libs/aitools/installer/installer.go index 29806cc419..eb627ad3ed 100644 --- a/libs/aitools/installer/installer.go +++ b/libs/aitools/installer/installer.go @@ -4,6 +4,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "encoding/json" "errors" "fmt" "io" @@ -12,9 +13,11 @@ import ( "net/http" "os" "path/filepath" + "runtime" "slices" "strings" "sync" + "syscall" "time" "github.com/databricks/cli/internal/build" @@ -76,11 +79,13 @@ func stateRepoDir(state *InstallState, name string) string { // It is a package-level var so tests can replace it with a mock. var fetchFileFn func(ctx context.Context, ref, repoDir, skillName, filePath string) ([]byte, error) = fetchSkillFile -// latestSkillsRef is the ref used when nothing pins a version. It tracks the -// skills repo's default branch, the same content plugin agents receive (their -// marketplace clones the default branch), so skills and plugin agents stay in -// sync by default. -const latestSkillsRef = "main" +// renamePathFn is the function used to move paths into place. Tests replace it +// to exercise cross-device fallback paths without depending on host mounts. +var renamePathFn = os.Rename + +// latestSkillsReleaseRefFn resolves the skills repo's latest release tag. Tests +// replace it so default ref resolution stays deterministic and offline. +var latestSkillsReleaseRefFn = fetchLatestSkillsReleaseRef // skillsLatestSentinel is the value cli-compat.json uses for the skills version // to mean "track latest" rather than pinning a specific release. @@ -89,9 +94,9 @@ const skillsLatestSentinel = clicompat.AgentSkillsLatest // GetSkillsRef returns the skills repo ref to use and whether it was explicitly // pinned (as opposed to tracking latest). // -// By default we track the latest skills. cli-compat.json is the remote control -// for this: it normally reports "latest" (so skills match what plugin agents -// install), but if a future change makes a skills release incompatible with +// By default we track the latest skills release tag, not the skills repo's main +// branch. cli-compat.json is the remote control for this: it normally reports +// "latest", but if a future change makes a skills release incompatible with // older CLIs, it can be edited remotely to pin those CLIs to the last compatible // version with no CLI release. DATABRICKS_SKILLS_REF overrides everything for // eval/reproducibility. (AppKit version resolution is unaffected; cli-compat @@ -102,23 +107,71 @@ func GetSkillsRef(ctx context.Context) (ref string, explicit bool, err error) { } v, err := clicompat.ResolveAgentSkillsVersion(ctx) if err != nil { - // If compatibility can't be resolved (offline, parse error), track latest - // rather than failing the install. - log.Debugf(ctx, "could not resolve skills compatibility, tracking latest: %v", err) - return latestSkillsRef, false, nil + log.Debugf(ctx, "could not resolve skills compatibility, resolving latest release: %v", err) + return resolveLatestSkillsReleaseRef(ctx) } if v == "" || v == skillsLatestSentinel { - return latestSkillsRef, false, nil + return resolveLatestSkillsReleaseRef(ctx) } return "v" + v, true, nil } -// DisplaySkillsVersion renders a skills ref for humans: the latest sentinel -// becomes "latest"; a pinned tag drops its leading "v". -func DisplaySkillsVersion(ref string) string { - if ref == latestSkillsRef { - return "latest" +func resolveLatestSkillsReleaseRef(ctx context.Context) (ref string, explicit bool, err error) { + ref, err = latestSkillsReleaseRefFn(ctx) + if err == nil { + return ref, false, nil + } + + embedded, embeddedErr := clicompat.ResolveEmbeddedAgentSkillsVersion() + if embeddedErr == nil && embedded != "" && embedded != skillsLatestSentinel { + log.Warnf(ctx, "Could not resolve latest skills release (%v); falling back to embedded skills version %s", err, embedded) + return "v" + embedded, false, nil + } + if embeddedErr != nil { + return "", false, fmt.Errorf("resolve latest skills release: %w (embedded fallback failed: %v)", err, embeddedErr) + } + return "", false, fmt.Errorf("resolve latest skills release: %w", err) +} + +func fetchLatestSkillsReleaseRef(ctx context.Context) (string, error) { + 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 { + return "", fmt.Errorf("failed to create request: %w", err) + } + + resp, err := httpClient().Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("HTTP %d fetching latest skills release", resp.StatusCode) + } + + var payload struct { + TagName string `json:"tag_name"` + } + if err := json.NewDecoder(io.LimitReader(resp.Body, 1<<20)).Decode(&payload); err != nil { + return "", fmt.Errorf("parse latest skills release: %w", err) + } + tag := strings.TrimSpace(payload.TagName) + if tag == "" { + return "", errors.New("latest skills release has empty tag_name") } + if !strings.HasPrefix(tag, "v") { + tag = "v" + tag + } + if !semver.IsValid(tag) { + return "", fmt.Errorf("latest skills release has invalid tag %q", payload.TagName) + } + return tag, nil +} + +// DisplaySkillsVersion renders a skills ref for humans: a pinned tag drops its +// leading "v". +func DisplaySkillsVersion(ref string) string { return strings.TrimPrefix(ref, "v") } @@ -179,15 +232,19 @@ func fetchSkillFile(ctx context.Context, ref, repoDir, skillName, filePath strin resp, err := httpClient().Do(req) if err != nil { - return nil, fmt.Errorf("failed to fetch %s: %w", filePath, err) + return nil, err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed to fetch %s: HTTP %d", filePath, resp.StatusCode) + return nil, fmt.Errorf("HTTP %d", resp.StatusCode) } - return io.ReadAll(resp.Body) + data, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response: %w", err) + } + return data, nil } // FetchSkillsManifestWithFallback fetches the skills manifest at the given ref. @@ -219,13 +276,14 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent return err } cmdio.LogString(ctx, "Using skills version "+DisplaySkillsVersion(ref)) + cmdio.LogString(ctx, "Fetching skills manifest...") manifest, ref, err := FetchSkillsManifestWithFallback(ctx, src, ref, !explicit) if err != nil { return err } if opts.IncludeExperimental && !manifestHasExperimental(manifest) { - log.Warnf(ctx, "--experimental was set but the manifest at %s exposes no experimental skills. Set DATABRICKS_SKILLS_REF to a release that includes them (or =main for the latest).", ref) + log.Warnf(ctx, "--experimental was set but the manifest at %s exposes no experimental skills. Set DATABRICKS_SKILLS_REF to a release or ref that includes them.", ref) } scope := opts.Scope @@ -522,6 +580,7 @@ type installParams struct { func installSkillForAgents(ctx context.Context, skillName string, meta SkillMeta, detectedAgents []*agents.Agent, params installParams) (map[string]FileRecord, error) { canonicalDir := filepath.Join(params.baseDir, skillName) + cmdio.LogString(ctx, fmt.Sprintf("Downloading %s...", skillName)) records, err := installSkillToDir(ctx, params.ref, meta.RepoDir, meta.SourceName, canonicalDir, meta.Files) if err != nil { return nil, err @@ -529,6 +588,7 @@ func installSkillForAgents(ctx context.Context, skillName string, meta SkillMeta // For project scope, always symlink. For global, symlink when multiple agents. useSymlinks := params.scope == ScopeProject || len(detectedAgents) > 1 + cmdio.LogString(ctx, fmt.Sprintf("Exposing %s to %d %s...", skillName, len(detectedAgents), agentNoun(len(detectedAgents)))) for _, agent := range detectedAgents { agentSkillDir, err := agentSkillsDirForScope(ctx, agent, params.scope, params.cwd) @@ -574,6 +634,13 @@ func installSkillForAgents(ctx context.Context, skillName string, meta SkillMeta return records, nil } +func agentNoun(n int) string { + if n == 1 { + return "agent" + } + return "agents" +} + // agentSkillsDirForScope returns the agent's skills directory for the given scope. func agentSkillsDirForScope(ctx context.Context, agent *agents.Agent, scope, cwd string) (string, error) { if scope == ScopeProject { @@ -613,7 +680,7 @@ func backupThirdPartySkill(ctx context.Context, destDir, canonicalDir, skillName } backupDest := filepath.Join(backupDir, skillName) - if err := os.Rename(destDir, backupDest); err != nil { + if err := movePathWithCrossDeviceFallback(destDir, backupDest); err != nil { return fmt.Errorf("failed to move existing skill: %w", err) } @@ -621,19 +688,52 @@ func backupThirdPartySkill(ctx context.Context, destDir, canonicalDir, skillName return nil } +func movePathWithCrossDeviceFallback(src, dest string) error { + if err := renamePathFn(src, dest); err != nil { + if !isCrossDeviceRenameError(err) { + return err + } + if err := copyPath(src, dest); err != nil { + return err + } + if err := os.RemoveAll(src); err != nil { + return fmt.Errorf("failed to remove original path after copy: %w", err) + } + } + return nil +} + +func isCrossDeviceRenameError(err error) bool { + if errors.Is(err, syscall.EXDEV) { + return true + } + if runtime.GOOS != "windows" { + return false + } + + errno, ok := errors.AsType[syscall.Errno](err) + return ok && errno == syscall.Errno(17) // ERROR_NOT_SAME_DEVICE +} + // installSkillToDir downloads a skill's files into destDir and returns a // FileRecord per file (keyed by "/", forward slashes) capturing // the sha256 of the bytes written, so a later update can prune only the files we // wrote that the user hasn't modified. func installSkillToDir(ctx context.Context, ref, repoDir, skillName, destDir string, files []string) (map[string]FileRecord, error) { - // remove existing skill directory for clean install - if err := os.RemoveAll(destDir); err != nil { - return nil, fmt.Errorf("failed to remove existing skill: %w", err) + parentDir := filepath.Dir(destDir) + if err := os.MkdirAll(parentDir, 0o755); err != nil { + return nil, fmt.Errorf("failed to create skills directory: %w", err) } - - if err := os.MkdirAll(destDir, 0o755); err != nil { - return nil, fmt.Errorf("failed to create directory: %w", err) + tmpDir, err := os.MkdirTemp(parentDir, "."+filepath.Base(destDir)+"-*.tmp") + if err != nil { + return nil, fmt.Errorf("failed to create temporary skill directory: %w", err) } + cleanupTmp := true + defer func() { + if cleanupTmp { + _ = os.RemoveAll(tmpDir) + } + }() var mu sync.Mutex records := make(map[string]FileRecord, len(files)) @@ -647,9 +747,9 @@ func installSkillToDir(ctx context.Context, ref, repoDir, skillName, destDir str g.Go(func() error { content, err := fetchFileFn(gctx, ref, repoDir, skillName, file) if err != nil { - return err + return fmt.Errorf("failed to fetch %s/%s/%s at %s: %w", repoDir, skillName, filepath.ToSlash(file), ref, err) } - destPath := filepath.Join(destDir, file) + destPath := filepath.Join(tmpDir, file) if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { return fmt.Errorf("failed to create directory: %w", err) } @@ -670,9 +770,71 @@ func installSkillToDir(ctx context.Context, ref, repoDir, skillName, destDir str if err := g.Wait(); err != nil { return nil, err } + + if err := replaceDir(destDir, tmpDir); err != nil { + return nil, err + } + cleanupTmp = false return records, nil } +func replaceDir(destDir, tmpDir string) error { + parentDir := filepath.Dir(destDir) + base := filepath.Base(destDir) + + var backupParent string + var backupDest string + if _, err := os.Lstat(destDir); err == nil { + var mkErr error + backupParent, mkErr = os.MkdirTemp(parentDir, "."+base+"-backup-*.tmp") + if mkErr != nil { + return fmt.Errorf("failed to create backup directory: %w", mkErr) + } + backupDest = filepath.Join(backupParent, base) + if err := renamePathFn(destDir, backupDest); err != nil { + _ = os.RemoveAll(backupParent) + return fmt.Errorf("failed to move existing skill aside: %w", err) + } + } else if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("failed to inspect existing skill: %w", err) + } + + if err := renamePathFn(tmpDir, destDir); err != nil { + if backupDest != "" { + if restoreErr := renamePathFn(backupDest, destDir); restoreErr != nil { + return fmt.Errorf("failed to move skill into place: %w (also failed to restore existing skill: %v)", err, restoreErr) + } + } + return fmt.Errorf("failed to move skill into place: %w", err) + } + + if backupParent != "" { + _ = os.RemoveAll(backupParent) + } + return nil +} + +func copyPath(src, dest string) error { + info, err := os.Lstat(src) + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + target, err := os.Readlink(src) + if err != nil { + return fmt.Errorf("failed to read symlink: %w", err) + } + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return err + } + return os.Symlink(target, dest) + } + if info.IsDir() { + return copyDir(src, dest) + } + return copyFile(src, dest, info.Mode()) +} + // copyDir copies all files from src to dest, recreating the directory structure. func copyDir(src, dest string) error { if err := os.RemoveAll(dest); err != nil { @@ -705,6 +867,17 @@ func copyDir(src, dest string) error { }) } +func copyFile(src, dest string, mode fs.FileMode) error { + data, err := os.ReadFile(src) + if err != nil { + return fmt.Errorf("failed to read %s: %w", src, err) + } + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return err + } + return os.WriteFile(dest, data, mode.Perm()) +} + func createSymlink(source, dest string) error { // ensure parent directory exists if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { diff --git a/libs/aitools/installer/installer_test.go b/libs/aitools/installer/installer_test.go index 1b6847b9ab..f32f737dc0 100644 --- a/libs/aitools/installer/installer_test.go +++ b/libs/aitools/installer/installer_test.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "sync" + "syscall" "testing" "time" @@ -63,6 +64,15 @@ func setupFetchMock(t *testing.T) { } } +func stubLatestSkillsReleaseRef(t *testing.T, ref string, err error) { + t.Helper() + orig := latestSkillsReleaseRefFn + t.Cleanup(func() { latestSkillsReleaseRefFn = orig }) + latestSkillsReleaseRefFn = func(context.Context) (string, error) { + return ref, err + } +} + func testAgent(tmpHome string) *agents.Agent { return &agents.Agent{ Name: "test-agent", @@ -149,6 +159,51 @@ func TestBackupThirdPartySkillRegularDir(t *testing.T) { require.NoError(t, os.RemoveAll(filepath.Dir(filepath.Dir(matches[0])))) } +func TestBackupThirdPartySkillCrossDeviceFallback(t *testing.T) { + ctx := cmdio.MockDiscard(t.Context()) + tmp := t.TempDir() + skillName := "databricks-cross-device" + + cleanupBackups := func() { + matches, _ := filepath.Glob(filepath.Join(os.TempDir(), "databricks-skill-backup-"+skillName+"-*")) + for _, match := range matches { + _ = os.RemoveAll(match) + } + } + cleanupBackups() + t.Cleanup(cleanupBackups) + + destDir := filepath.Join(tmp, "agent", "skills", skillName) + require.NoError(t, os.MkdirAll(destDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(destDir, "custom.md"), []byte("custom"), 0o644)) + + orig := renamePathFn + t.Cleanup(func() { renamePathFn = orig }) + renameCalled := false + renamePathFn = func(oldpath, newpath string) error { + if oldpath == destDir { + renameCalled = true + return &os.LinkError{Op: "rename", Old: oldpath, New: newpath, Err: syscall.EXDEV} + } + return os.Rename(oldpath, newpath) + } + + err := backupThirdPartySkill(ctx, destDir, "/some/canonical", skillName, "Test Agent") + require.NoError(t, err) + assert.True(t, renameCalled) + + _, err = os.Stat(destDir) + assert.ErrorIs(t, err, fs.ErrNotExist) + + matches, err := filepath.Glob(filepath.Join(os.TempDir(), "databricks-skill-backup-"+skillName+"-*", skillName, "custom.md")) + require.NoError(t, err) + require.Len(t, matches, 1) + + content, err := os.ReadFile(matches[0]) + require.NoError(t, err) + assert.Equal(t, "custom", string(content)) +} + func TestBackupThirdPartySkillSymlinkToOtherTarget(t *testing.T) { ctx := cmdio.MockDiscard(t.Context()) tmp := t.TempDir() @@ -302,6 +357,59 @@ func TestInstallSkillToDirCancelsInFlightFetchesOnError(t *testing.T) { } } +func TestInstallSkillToDirFetchErrorIncludesContextAndCleansTemp(t *testing.T) { + ctx := cmdio.MockDiscard(t.Context()) + + orig := fetchFileFn + t.Cleanup(func() { fetchFileFn = orig }) + + fetchErr := errors.New("HTTP 404") + fetchFileFn = func(_ context.Context, _, _, _, _ string) ([]byte, error) { + return nil, fetchErr + } + + destDir := filepath.Join(t.TempDir(), "databricks-test") + _, err := installSkillToDir(ctx, testSkillsRef, stableSkillsRepoPath, "databricks-test", destDir, []string{"assets/databricks.png"}) + require.ErrorIs(t, err, fetchErr) + assert.Contains(t, err.Error(), stableSkillsRepoPath+"/databricks-test/assets/databricks.png") + assert.Contains(t, err.Error(), testSkillsRef) + + _, statErr := os.Stat(destDir) + assert.ErrorIs(t, statErr, fs.ErrNotExist) + + matches, globErr := filepath.Glob(filepath.Join(filepath.Dir(destDir), "."+filepath.Base(destDir)+"-*.tmp")) + require.NoError(t, globErr) + assert.Empty(t, matches) +} + +func TestInstallSkillToDirFetchFailureKeepsExistingSkill(t *testing.T) { + ctx := cmdio.MockDiscard(t.Context()) + + orig := fetchFileFn + t.Cleanup(func() { fetchFileFn = orig }) + + fetchErr := errors.New("HTTP 404") + fetchFileFn = func(_ context.Context, _, _, _, filePath string) ([]byte, error) { + if filePath == "assets/databricks.png" { + return nil, fetchErr + } + return []byte("new"), nil + } + + destDir := filepath.Join(t.TempDir(), "databricks-test") + require.NoError(t, os.MkdirAll(destDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(destDir, "SKILL.md"), []byte("old"), 0o644)) + + _, err := installSkillToDir(ctx, testSkillsRef, stableSkillsRepoPath, "databricks-test", destDir, []string{"SKILL.md", "assets/databricks.png"}) + require.ErrorIs(t, err, fetchErr) + + content, err := os.ReadFile(filepath.Join(destDir, "SKILL.md")) + require.NoError(t, err) + assert.Equal(t, "old", string(content)) + _, err = os.Stat(filepath.Join(destDir, "assets", "databricks.png")) + assert.ErrorIs(t, err, fs.ErrNotExist) +} + // --- InstallSkillsForAgents tests --- func TestInstallSkillsForAgentsWritesState(t *testing.T) { @@ -331,6 +439,9 @@ func TestInstallSkillsForAgentsWritesState(t *testing.T) { assert.NotEmpty(t, state.Files["databricks-sql/SKILL.md"].SHA256) assert.Equal(t, testSkillsRef, state.Files["databricks-sql/SKILL.md"].Origin) + assert.Contains(t, stderr.String(), "Fetching skills manifest...") + assert.Contains(t, stderr.String(), "Downloading databricks-sql...") + assert.Contains(t, stderr.String(), "Exposing databricks-sql to 1 agent...") assert.Contains(t, stderr.String(), "Installed 2 skills.") } @@ -943,17 +1054,29 @@ func TestSupportsProjectScopeSetCorrectly(t *testing.T) { } func TestGetSkillsRefDefaultsToLatest(t *testing.T) { - // cli-compat reports "latest", so we track the repo's default branch (the - // same content plugin agents install). + // cli-compat reports "latest", so we resolve it to the skills repo's + // latest release tag rather than tracking the default branch. t.Setenv("DATABRICKS_SKILLS_REF", "") withCachedCompat(t, skillsLatestSentinel) + stubLatestSkillsReleaseRef(t, "v0.2.8", nil) ref, explicit, err := GetSkillsRef(t.Context()) require.NoError(t, err) - assert.Equal(t, latestSkillsRef, ref) + assert.Equal(t, "v0.2.8", ref) assert.False(t, explicit, "tracking latest is not an explicit pin") } +func TestGetSkillsRefLatestReleaseFallsBackToEmbeddedPin(t *testing.T) { + t.Setenv("DATABRICKS_SKILLS_REF", "") + withCachedCompat(t, skillsLatestSentinel) + stubLatestSkillsReleaseRef(t, "", errors.New("network down")) + + ref, explicit, err := GetSkillsRef(t.Context()) + require.NoError(t, err) + assert.Equal(t, "v0.2.9", ref) + assert.False(t, explicit, "an embedded fallback is not a user pin") +} + func TestGetSkillsRefEnvEscapeHatch(t *testing.T) { t.Setenv("DATABRICKS_SKILLS_REF", "v9.9.9") @@ -976,9 +1099,9 @@ func TestGetSkillsRefPinnedByCliCompat(t *testing.T) { } func TestDisplaySkillsVersion(t *testing.T) { - assert.Equal(t, "latest", DisplaySkillsVersion(latestSkillsRef)) assert.Equal(t, "0.2.5", DisplaySkillsVersion("v0.2.5")) assert.Equal(t, "0.2.5", DisplaySkillsVersion("0.2.5")) + assert.Equal(t, "latest", DisplaySkillsVersion("latest")) } // withCachedCompat pre-populates the cli-compat cache so resolution is offline diff --git a/libs/aitools/installer/installer_windows_test.go b/libs/aitools/installer/installer_windows_test.go new file mode 100644 index 0000000000..4b1528b851 --- /dev/null +++ b/libs/aitools/installer/installer_windows_test.go @@ -0,0 +1,59 @@ +//go:build windows + +package installer + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/databricks/cli/libs/cmdio" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBackupThirdPartySkillWindowsNotSameDeviceFallback(t *testing.T) { + ctx := cmdio.MockDiscard(t.Context()) + tmp := t.TempDir() + skillName := "databricks-windows-cross-device" + + cleanupBackups := func() { + matches, _ := filepath.Glob(filepath.Join(os.TempDir(), "databricks-skill-backup-"+skillName+"-*")) + for _, match := range matches { + _ = os.RemoveAll(match) + } + } + cleanupBackups() + t.Cleanup(cleanupBackups) + + destDir := filepath.Join(tmp, "agent", "skills", skillName) + require.NoError(t, os.MkdirAll(destDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(destDir, "custom.md"), []byte("custom"), 0o644)) + + orig := renamePathFn + t.Cleanup(func() { renamePathFn = orig }) + renameCalled := false + renamePathFn = func(oldpath, newpath string) error { + if oldpath == destDir { + renameCalled = true + return &os.LinkError{Op: "rename", Old: oldpath, New: newpath, Err: syscall.Errno(17)} + } + return os.Rename(oldpath, newpath) + } + + err := backupThirdPartySkill(ctx, destDir, "/some/canonical", skillName, "Test Agent") + require.NoError(t, err) + assert.True(t, renameCalled) + + _, err = os.Stat(destDir) + assert.ErrorIs(t, err, os.ErrNotExist) + + matches, err := filepath.Glob(filepath.Join(os.TempDir(), "databricks-skill-backup-"+skillName+"-*", skillName, "custom.md")) + require.NoError(t, err) + require.Len(t, matches, 1) + + content, err := os.ReadFile(matches[0]) + require.NoError(t, err) + assert.Equal(t, "custom", string(content)) +} diff --git a/libs/clicompat/clicompat.go b/libs/clicompat/clicompat.go index 2a8a60d0c9..e0daada0bc 100644 --- a/libs/clicompat/clicompat.go +++ b/libs/clicompat/clicompat.go @@ -419,7 +419,7 @@ func fetchRemote(ctx context.Context) (Manifest, error) { // AgentSkillsLatest is the sentinel value the skills field may take instead of a // concrete version, meaning "track the latest skills" rather than pinning a -// release. The aitools installer maps it to the skills repo's default branch. +// release. The aitools installer maps it to the skills repo's latest release tag. const AgentSkillsLatest = "latest" func parseManifest(data []byte) (Manifest, error) { From 895118ab67bd9afb5a60cff62488b78790cc75eb Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 30 Jun 2026 11:58:14 +0200 Subject: [PATCH 2/3] Fix aitools plugin lifecycle scoping --- libs/aitools/installer/plugin.go | 86 +++++++++++++++++++++--- libs/aitools/installer/plugin_test.go | 96 ++++++++++++++++++++++++++- libs/aitools/installer/update.go | 4 +- 3 files changed, 173 insertions(+), 13 deletions(-) diff --git a/libs/aitools/installer/plugin.go b/libs/aitools/installer/plugin.go index 9554b93a14..13fc9475e3 100644 --- a/libs/aitools/installer/plugin.go +++ b/libs/aitools/installer/plugin.go @@ -94,6 +94,18 @@ func installTarget(spec *agents.PluginSpec) string { return spec.ID + "@" + spec.Marketplace } +func recordedInstallTarget(agent *agents.Agent, rec PluginRecord) string { + plugin := rec.Plugin + if plugin == "" { + plugin = agent.Plugin.ID + } + marketplace := rec.Marketplace + if marketplace == "" { + marketplace = agent.Plugin.Marketplace + } + return plugin + "@" + marketplace +} + // marketplaceAddArgs builds the `plugin marketplace add ` argv (sans binary). func marketplaceAddArgs(spec *agents.PluginSpec) []string { return []string{"plugin", "marketplace", "add", spec.Source} @@ -118,6 +130,24 @@ func marketplaceRemoveArgs(spec *agents.PluginSpec) []string { return []string{"plugin", "marketplace", "remove", spec.Marketplace} } +func marketplaceRemoveArgsForRecord(agent *agents.Agent, rec PluginRecord) []string { + marketplace := rec.Marketplace + if marketplace == "" { + marketplace = agent.Plugin.Marketplace + } + return []string{"plugin", "marketplace", "remove", marketplace} +} + +// marketplaceUpdateArgs builds the marketplace refresh command for agents whose +// plugin CLI supports it. Claude's official marketplace can be stale locally, +// causing install/update to report that a published plugin does not exist. +func marketplaceUpdateArgs(agent *agents.Agent) []string { + if agent.Name == agents.NameClaudeCode { + return []string{"plugin", "marketplace", "update"} + } + return nil +} + // pluginInstallArgs builds the per-agent install argv (sans binary). Codex uses // `plugin add`; Claude is the only agent that accepts `--scope`. func pluginInstallArgs(agent *agents.Agent, scope string) []string { @@ -138,14 +168,19 @@ func pluginInstallArgs(agent *agents.Agent, scope string) []string { // pluginUpdateSteps builds the ordered per-agent update argv sets (sans binary). // Codex updates in two steps: refresh the marketplace, then re-add. -func pluginUpdateSteps(agent *agents.Agent) [][]string { - target := installTarget(agent.Plugin) +func pluginUpdateSteps(agent *agents.Agent, rec PluginRecord) [][]string { + target := recordedInstallTarget(agent, rec) switch agent.Name { case agents.NameCodex: return [][]string{ {"plugin", "marketplace", "upgrade"}, {"plugin", "add", target}, } + case agents.NameClaudeCode: + return [][]string{ + marketplaceUpdateArgs(agent), + appendScopeArg([]string{"plugin", "update", target}, rec.Scope), + } default: return [][]string{{"plugin", "update", target}} } @@ -153,16 +188,32 @@ func pluginUpdateSteps(agent *agents.Agent) [][]string { // pluginUninstallArgs builds the per-agent uninstall argv (sans binary). // Codex removes with `plugin remove`; the others use `plugin uninstall`. -func pluginUninstallArgs(agent *agents.Agent) []string { - target := installTarget(agent.Plugin) +func pluginUninstallArgs(agent *agents.Agent, rec PluginRecord) []string { + target := recordedInstallTarget(agent, rec) switch agent.Name { case agents.NameCodex: return []string{"plugin", "remove", target} + case agents.NameClaudeCode: + return appendScopeArg([]string{"plugin", "uninstall", target}, rec.Scope) default: return []string{"plugin", "uninstall", target} } } +func pluginDisableArgs(agent *agents.Agent, rec PluginRecord) []string { + if agent.Name != agents.NameClaudeCode { + return nil + } + return appendScopeArg([]string{"plugin", "disable", recordedInstallTarget(agent, rec)}, rec.Scope) +} + +func appendScopeArg(args []string, scope string) []string { + if scope == "" { + return args + } + return append(args, "--scope", scope) +} + // probePluginCLI resolves the agent's binary and confirms its CLI exposes the // plugin subcommand, so we don't register a marketplace on a CLI that can't // install plugins. Returns the resolved absolute path. @@ -205,6 +256,16 @@ func InstallPluginForAgent(ctx context.Context, agent *agents.Agent, nativeScope _, addErr := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, marketplaceAddArgs(agent.Plugin))) installedMarketplace = addErr == nil && !alreadyPresent } + if args := marketplaceUpdateArgs(agent); args != nil { + if _, err := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, args)); err != nil { + if installedMarketplace { + if _, rmErr := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, marketplaceRemoveArgs(agent.Plugin))); rmErr != nil { + log.Warnf(ctx, "%s plugin marketplace refresh failed and the marketplace could not be de-registered: %v", agent.DisplayName, rmErr) + } + } + return PluginRecord{}, &BlockedError{Agent: agent.Name, Reason: ReasonInstallFailed, Detail: stderrOf(err)} + } + } if _, err := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, pluginInstallArgs(agent, nativeScope))); err != nil { // Roll back a marketplace we just added so a failed install doesn't @@ -229,7 +290,7 @@ func InstallPluginForAgent(ctx context.Context, agent *agents.Agent, nativeScope // UpdatePluginForAgent updates the plugin through the agent's own CLI. The // plugin's own update handles content the release dropped, so there is no // per-skill prune for plugin agents. -func UpdatePluginForAgent(ctx context.Context, agent *agents.Agent) error { +func UpdatePluginForAgent(ctx context.Context, agent *agents.Agent, rec PluginRecord) error { if agent.Plugin == nil { return &BlockedError{Agent: agent.Name, Reason: ReasonNoPlugin} } @@ -237,7 +298,7 @@ func UpdatePluginForAgent(ctx context.Context, agent *agents.Agent) error { if err != nil { return &BlockedError{Agent: agent.Name, Reason: ReasonCLINotOnPath, Detail: err.Error()} } - for _, args := range pluginUpdateSteps(agent) { + for _, args := range pluginUpdateSteps(agent, rec) { if _, err := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, args)); err != nil { return &BlockedError{Agent: agent.Name, Reason: ReasonInstallFailed, Detail: stderrOf(err)} } @@ -262,19 +323,28 @@ func UninstallPluginForAgent(ctx context.Context, agent *agents.Agent, rec Plugi if err != nil { return &BlockedError{Agent: agent.Name, Reason: ReasonCLINotOnPath, Detail: err.Error()} } - if _, err := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, pluginUninstallArgs(agent))); err != nil { + if args := pluginDisableArgs(agent, rec); args != nil { + if _, err := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, args)); err != nil && !claudeAlreadyDisabled(err) { + return &BlockedError{Agent: agent.Name, Reason: ReasonInstallFailed, Detail: stderrOf(err)} + } + } + if _, err := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, pluginUninstallArgs(agent, rec))); err != nil { return &BlockedError{Agent: agent.Name, Reason: ReasonInstallFailed, Detail: stderrOf(err)} } // Never de-register a built-in marketplace (empty Source, e.g. Claude's // claude-plugins-official): it is shared infrastructure we did not add. if rec.InstalledMarketplace && !keepMarketplace && agent.Plugin.Source != "" { - if _, err := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, marketplaceRemoveArgs(agent.Plugin))); err != nil { + if _, err := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, marketplaceRemoveArgsForRecord(agent, rec))); err != nil { log.Warnf(ctx, "Removed the %s plugin but could not de-register its marketplace (remove it manually if needed): %v", agent.DisplayName, stderrOf(err)) } } return nil } +func claudeAlreadyDisabled(err error) bool { + return strings.Contains(strings.ToLower(stderrOf(err)), "already disabled") +} + // RecordPluginInstalls persists plugin install records into the state file for // the given CLI scope (global or project), creating state if none exists. ref // is the resolved skills release the install corresponds to. diff --git a/libs/aitools/installer/plugin_test.go b/libs/aitools/installer/plugin_test.go index b4c146284b..15415a6106 100644 --- a/libs/aitools/installer/plugin_test.go +++ b/libs/aitools/installer/plugin_test.go @@ -61,6 +61,7 @@ func TestInstallPluginForAgentClaudeSuccess(t *testing.T) { cmds := stub.Commands() assert.Contains(t, cmds, "claude plugin --help") assert.Contains(t, cmds, "claude plugin marketplace add databricks/databricks-agent-skills") + assert.Contains(t, cmds, "claude plugin marketplace update") assert.Contains(t, cmds, "claude plugin install databricks@databricks-agent-skills --scope user") } @@ -87,6 +88,7 @@ func TestInstallPluginForAgentBuiltinMarketplace(t *testing.T) { for _, c := range cmds { assert.NotContains(t, c, "marketplace add", "must not register a built-in marketplace") } + assert.Contains(t, cmds, "claude plugin marketplace update") assert.Contains(t, cmds, "claude plugin install databricks@claude-plugins-official --scope user") } @@ -166,12 +168,27 @@ func TestInstallPluginRollsBackMarketplaceOnInstallFailure(t *testing.T) { assert.Contains(t, stub.Commands(), "claude plugin marketplace remove databricks-agent-skills") } +func TestInstallPluginRollsBackMarketplaceOnRefreshFailure(t *testing.T) { + stubAgentLookPath(t, true) + ctx, stub := process.WithStub(t.Context()) + stub.WithCallback(func(*exec.Cmd) error { return nil }) + // Marketplace absent (empty list) so we add it; then the refresh fails. + stub.WithFailureFor("plugin marketplace update", errors.New("boom")) + + _, err := InstallPluginForAgent(ctx, claudeAgent(), "user", "v0.2.6") + var be *BlockedError + require.ErrorAs(t, err, &be) + assert.Equal(t, ReasonInstallFailed, be.Reason) + // We added the marketplace, so a failed refresh must de-register it again. + assert.Contains(t, stub.Commands(), "claude plugin marketplace remove databricks-agent-skills") +} + func TestUpdatePluginForAgentCodexTwoStep(t *testing.T) { stubAgentLookPath(t, true) ctx, stub := process.WithStub(t.Context()) stub.WithCallback(func(*exec.Cmd) error { return nil }) - require.NoError(t, UpdatePluginForAgent(ctx, codexAgent())) + require.NoError(t, UpdatePluginForAgent(ctx, codexAgent(), PluginRecord{Marketplace: "databricks-agent-skills", Plugin: "databricks", Scope: "user"})) cmds := stub.Commands() assert.Contains(t, cmds, "codex plugin marketplace upgrade") assert.Contains(t, cmds, "codex plugin add databricks@databricks-agent-skills") @@ -182,8 +199,25 @@ func TestUpdatePluginForAgentClaudeOneStep(t *testing.T) { ctx, stub := process.WithStub(t.Context()) stub.WithCallback(func(*exec.Cmd) error { return nil }) - require.NoError(t, UpdatePluginForAgent(ctx, claudeAgent())) - assert.Contains(t, stub.Commands(), "claude plugin update databricks@databricks-agent-skills") + require.NoError(t, UpdatePluginForAgent(ctx, claudeAgent(), PluginRecord{Marketplace: "databricks-agent-skills", Plugin: "databricks", Scope: "user"})) + cmds := stub.Commands() + assert.Contains(t, cmds, "claude plugin marketplace update") + assert.Contains(t, cmds, "claude plugin update databricks@databricks-agent-skills --scope user") +} + +func TestUninstallPluginClaudeAlreadyDisabledContinues(t *testing.T) { + stubAgentLookPath(t, true) + ctx, stub := process.WithStub(t.Context()) + stub.WithCallback(func(*exec.Cmd) error { return nil }) + stub.WithStderrFor("plugin disable", "Plugin databricks is already disabled in user scope"). + WithFailureFor("plugin disable", errors.New("exit status 1")) + + rec := PluginRecord{Marketplace: "databricks-agent-skills", Plugin: "databricks", Scope: "user"} + require.NoError(t, UninstallPluginForAgent(ctx, claudeAgent(), rec, false)) + + cmds := stub.Commands() + assert.Contains(t, cmds, "claude plugin disable databricks@databricks-agent-skills --scope user") + assert.Contains(t, cmds, "claude plugin uninstall databricks@databricks-agent-skills --scope user") } func TestUninstallPluginDeregistersMarketplaceWhenInstalledByUs(t *testing.T) { @@ -370,6 +404,62 @@ func TestUpdateInstalledPlugins(t *testing.T) { assert.Equal(t, "0.2.7", state.Plugins[agents.NameCopilot].Version) } +func TestUpdateInstalledPluginsUsesRecordedClaudeScope(t *testing.T) { + for _, scope := range []string{"project", "user", "local"} { + t.Run(scope, func(t *testing.T) { + setupTestHome(t) + stubAgentLookPath(t, true) + ctx, stub := process.WithStub(t.Context()) + stub.WithCallback(func(*exec.Cmd) error { return nil }) + + dir, err := GlobalSkillsDir(ctx) + require.NoError(t, err) + require.NoError(t, SaveState(dir, &InstallState{ + SchemaVersion: schemaVersionV2, + Release: "v0.2.6", + Plugins: map[string]PluginRecord{ + agents.NameClaudeCode: {Marketplace: "claude-plugins-official", Plugin: "databricks", Scope: scope, Version: "0.2.6"}, + }, + })) + + updated, err := UpdateInstalledPlugins(ctx, ScopeGlobal, "v0.2.7") + require.NoError(t, err) + require.Len(t, updated, 1) + + cmds := stub.Commands() + assert.Contains(t, cmds, "claude plugin marketplace update") + assert.Contains(t, cmds, "claude plugin update databricks@claude-plugins-official --scope "+scope) + }) + } +} + +func TestUninstallSkillsOptsUsesRecordedClaudeScope(t *testing.T) { + for _, scope := range []string{"project", "user", "local"} { + t.Run(scope, func(t *testing.T) { + setupTestHome(t) + stubAgentLookPath(t, true) + ctx, stub := process.WithStub(t.Context()) + stub.WithCallback(func(*exec.Cmd) error { return nil }) + ctx = cmdio.MockDiscard(ctx) + + dir, err := GlobalSkillsDir(ctx) + require.NoError(t, err) + require.NoError(t, SaveState(dir, &InstallState{ + SchemaVersion: schemaVersionV2, + Plugins: map[string]PluginRecord{ + agents.NameClaudeCode: {Marketplace: "claude-plugins-official", Plugin: "databricks", Scope: scope}, + }, + })) + + require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{Scope: ScopeGlobal})) + + cmds := stub.Commands() + assert.Contains(t, cmds, "claude plugin disable databricks@claude-plugins-official --scope "+scope) + assert.Contains(t, cmds, "claude plugin uninstall databricks@claude-plugins-official --scope "+scope) + }) + } +} + func TestUninstallPluginKeepMarketplaceFlag(t *testing.T) { stubAgentLookPath(t, true) ctx, stub := process.WithStub(t.Context()) diff --git a/libs/aitools/installer/update.go b/libs/aitools/installer/update.go index 0d27bb1dd4..0b1e30efcf 100644 --- a/libs/aitools/installer/update.go +++ b/libs/aitools/installer/update.go @@ -444,11 +444,11 @@ func UpdateInstalledPlugins(ctx context.Context, scope, ref string) ([]PluginUpd log.Warnf(ctx, "Skipping unknown agent %q in state", name) continue } - if err := UpdatePluginForAgent(ctx, agent); err != nil { + rec := state.Plugins[name] + if err := UpdatePluginForAgent(ctx, agent, rec); err != nil { log.Warnf(ctx, "Skipped %s: %v", agent.DisplayName, err) continue } - rec := state.Plugins[name] rec.Version = version state.Plugins[name] = rec updated = append(updated, PluginUpdate{Agent: agent.DisplayName, Version: version}) From 646c1724f8e3d8df799b8c959024110225aa7765 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 30 Jun 2026 12:07:09 +0200 Subject: [PATCH 3/3] Improve aitools command UX --- .../aitools/skills/update-prune/output.txt | 2 + cmd/aitools/install_test.go | 6 +- cmd/aitools/list.go | 47 +-- cmd/aitools/list_test.go | 48 ++- cmd/aitools/uninstall.go | 21 +- cmd/aitools/uninstall_test.go | 38 +++ cmd/aitools/update.go | 133 +++++++- cmd/aitools/update_test.go | 303 ++++++++++++++++++ cmd/aitools/version.go | 19 +- cmd/aitools/version_test.go | 32 +- libs/aitools/agents/agents.go | 10 +- libs/aitools/installer/cleanup.go | 49 ++- libs/aitools/installer/cleanup_test.go | 40 +++ libs/aitools/installer/installer_test.go | 2 +- libs/aitools/installer/plugin_test.go | 33 ++ libs/aitools/installer/uninstall.go | 121 ++++++- libs/aitools/installer/uninstall_test.go | 73 +++++ 17 files changed, 911 insertions(+), 66 deletions(-) diff --git a/acceptance/experimental/aitools/skills/update-prune/output.txt b/acceptance/experimental/aitools/skills/update-prune/output.txt index b6d9c3ed25..5cc1a8c5bd 100644 --- a/acceptance/experimental/aitools/skills/update-prune/output.txt +++ b/acceptance/experimental/aitools/skills/update-prune/output.txt @@ -15,6 +15,8 @@ Installed 2 skills. === update against a release where beta is gone: alpha updates, beta is pruned >>> DATABRICKS_SKILLS_REF=v2-ref [CLI] experimental aitools update --scope global Command "update" is deprecated, use "databricks aitools update" instead. +Installing databricks plugin for Claude Code... +Skipped Claude Code: claude-code: install-failed: ✘ Failed to install plugin "databricks@claude-plugins-official": Plugin "databricks" not found in marketplace "claude-plugins-official". Your local copy may be out of date — try `claude plugin marketplace update claude-plugins-official`. Downloading alpha... Exposing alpha to 1 agent... updated alpha v1.0.0 -> v2.0.0 diff --git a/cmd/aitools/install_test.go b/cmd/aitools/install_test.go index 415821829e..bd3394eae3 100644 --- a/cmd/aitools/install_test.go +++ b/cmd/aitools/install_test.go @@ -147,15 +147,15 @@ func TestAgentChoicesOnlyOffersActionableAgents(t *testing.T) { fakeBinsOnPath(t, "claude") ctx := cmdio.MockDiscard(t.Context()) - // Project scope: only Claude (plugin) and Cursor (skills) support it; the - // user-only plugin agents and files-only agents are not offered as choices. + // Project scope: only Claude (plugin) supports it; the user-only plugin + // agents and files-only agents are not offered as choices. choices := agentChoices(ctx, installer.ScopeProject, false) var names []string for _, c := range choices { names = append(names, c.agent.Name) } assert.Contains(t, names, agents.NameClaudeCode) - assert.Contains(t, names, agents.NameCursor) + assert.NotContains(t, names, agents.NameCursor) assert.NotContains(t, names, agents.NameCodex) assert.NotContains(t, names, agents.NameOpenCode) assert.NotContains(t, names, agents.NameCopilot) diff --git a/cmd/aitools/list.go b/cmd/aitools/list.go index 75bff4ecf0..b958413787 100644 --- a/cmd/aitools/list.go +++ b/cmd/aitools/list.go @@ -89,7 +89,8 @@ type agentEntry struct { // pluginInfo is the per-scope plugin record surfaced in list output. type pluginInfo struct { - Version string `json:"version,omitempty"` + Version string `json:"version,omitempty"` + NativeScope string `json:"native_scope,omitempty"` } type skillEntry struct { @@ -213,7 +214,7 @@ func buildAgentEntries(states map[string]*installer.InstallState) []agentEntry { installed := map[string]pluginInfo{} for scope, st := range states { if rec, ok := st.Plugins[a.Name]; ok { - installed[scope] = pluginInfo{Version: rec.Version} + installed[scope] = pluginInfo{Version: rec.Version, NativeScope: rec.Scope} } } if len(installed) > 0 { @@ -265,29 +266,31 @@ func renderListText(ctx context.Context, out listOutput, scope string) { } } - cmdio.LogString(ctx, "Available skills ("+versionToken(out.Release)+"):") - cmdio.LogString(ctx, "") - cmdio.LogString(ctx, renderSkillTable(stable, bothScopes)) - - if len(experimental) > 0 { - cmdio.LogString(ctx, "Experimental skills:") - cmdio.LogString(ctx, "") - cmdio.LogString(ctx, renderSkillTable(experimental, bothScopes)) - } - - cmdio.LogString(ctx, summaryLine(out, scope)) - if len(out.Agents) > 0 { + cmdio.LogString(ctx, "Plugin installs:") cmdio.LogString(ctx, "") var ab strings.Builder atw := tabwriter.NewWriter(&ab, 0, 4, 2, ' ', 0) fmt.Fprintln(atw, " AGENT\tSTATUS") for _, a := range out.Agents { - fmt.Fprintf(atw, " %s\t%s\n", a.Name, agentStatusLabel(a, out.Release)) + fmt.Fprintf(atw, " %s\t%s\n", agentDisplayName(a.Name), agentStatusLabel(a, out.Release)) } atw.Flush() cmdio.LogString(ctx, ab.String()) + cmdio.LogString(ctx, "") } + + cmdio.LogString(ctx, "Available raw skill directories ("+versionToken(out.Release)+"):") + cmdio.LogString(ctx, "") + cmdio.LogString(ctx, renderSkillTable(stable, bothScopes)) + + if len(experimental) > 0 { + cmdio.LogString(ctx, "Experimental skills:") + cmdio.LogString(ctx, "") + cmdio.LogString(ctx, renderSkillTable(experimental, bothScopes)) + } + + cmdio.LogString(ctx, summaryLine(out, scope)) } // renderSkillTable formats a NAME/VERSION/INSTALLED table for a group of skills. @@ -323,9 +326,9 @@ func agentStatusLabel(a agentEntry, release string) string { } if upToDate { - return "plugin · " + versionToken(version) + " · up to date" + return "databricks plugin · " + versionToken(version) + " · up to date" } - return "plugin · " + versionToken(version) + " · update available" + return "databricks plugin · " + versionToken(version) + " · update available" } func installedStatusFromEntry(s skillEntry, bothScopes bool) string { @@ -372,15 +375,15 @@ func summaryLine(out listOutput, scope string) string { // Mirror prior behavior: only print the dual-scope line when both // scopes have a state file; otherwise only mention the one that does. if g.loaded && p.loaded { - return fmt.Sprintf("%d/%d skills installed (global), %d/%d (project)", g.Installed, g.Total, p.Installed, p.Total) + return fmt.Sprintf("%d/%d raw skill directories installed (global), %d/%d (project)", g.Installed, g.Total, p.Installed, p.Total) } if p.loaded { - return fmt.Sprintf("%d/%d skills installed (project)", p.Installed, p.Total) + return fmt.Sprintf("%d/%d raw skill directories installed (project)", p.Installed, p.Total) } - return fmt.Sprintf("%d/%d skills installed (global)", g.Installed, g.Total) + return fmt.Sprintf("%d/%d raw skill directories installed (global)", g.Installed, g.Total) case pOK: - return fmt.Sprintf("%d/%d skills installed (project)", p.Installed, p.Total) + return fmt.Sprintf("%d/%d raw skill directories installed (project)", p.Installed, p.Total) default: - return fmt.Sprintf("%d/%d skills installed (global)", g.Installed, g.Total) + return fmt.Sprintf("%d/%d raw skill directories installed (global)", g.Installed, g.Total) } } diff --git a/cmd/aitools/list_test.go b/cmd/aitools/list_test.go index 40cc7a271b..c977ac1b5c 100644 --- a/cmd/aitools/list_test.go +++ b/cmd/aitools/list_test.go @@ -156,12 +156,12 @@ func TestBuildAgentEntries(t *testing.T) { require.Contains(t, byName, "claude-code") assert.True(t, byName["claude-code"].Managed) assert.Equal(t, "0.2.6", byName["claude-code"].Installed[installer.ScopeGlobal].Version) - assert.Equal(t, "plugin · v0.2.6 · up to date", agentStatusLabel(byName["claude-code"], "0.2.6")) + assert.Equal(t, "databricks plugin · v0.2.6 · up to date", agentStatusLabel(byName["claude-code"], "0.2.6")) require.Contains(t, byName, "codex") assert.True(t, byName["codex"].Managed) assert.Equal(t, "0.2.5", byName["codex"].Installed[installer.ScopeGlobal].Version) - assert.Equal(t, "plugin · v0.2.5 · update available", agentStatusLabel(byName["codex"], "0.2.6")) + assert.Equal(t, "databricks plugin · v0.2.5 · update available", agentStatusLabel(byName["codex"], "0.2.6")) // Cursor has no plugin, so it never appears as a plugin agent entry. assert.NotContains(t, byName, "cursor") @@ -194,7 +194,7 @@ func TestBuildAgentEntriesRecordsPerScopeVersions(t *testing.T) { // The renderer collapses the scopes and surfaces the stale one, rather than // hiding it behind the up-to-date scope. - assert.Equal(t, "plugin · v0.2.5 · update available", agentStatusLabel(cc, "0.2.6")) + assert.Equal(t, "databricks plugin · v0.2.5 · update available", agentStatusLabel(cc, "0.2.6")) } func TestRenderListJSONScopeFiltersSummary(t *testing.T) { @@ -282,7 +282,7 @@ func TestSummaryLinePreservesStatePresence(t *testing.T) { installer.ScopeProject: {Installed: 0, Total: 1, loaded: true}, }, }, - want: "0/1 skills installed (global), 0/1 (project)", + want: "0/1 raw skill directories installed (global), 0/1 (project)", }, { name: "only project state loaded", @@ -295,7 +295,7 @@ func TestSummaryLinePreservesStatePresence(t *testing.T) { installer.ScopeProject: {Installed: 0, Total: 1, loaded: true}, }, }, - want: "0/1 skills installed (project)", + want: "0/1 raw skill directories installed (project)", }, { name: "only global state loaded", @@ -308,7 +308,7 @@ func TestSummaryLinePreservesStatePresence(t *testing.T) { installer.ScopeProject: {Installed: 0, Total: 1}, }, }, - want: "0/1 skills installed (global)", + want: "0/1 raw skill directories installed (global)", }, } @@ -342,7 +342,7 @@ func TestRenderListTextUsesLoadedStateForScopeLabels(t *testing.T) { got := stderr.String() assert.Contains(t, got, "v1.0.0 (up to date) (global)") - assert.Contains(t, got, "1/1 skills installed (global), 0/1 (project)") + assert.Contains(t, got, "1/1 raw skill directories installed (global), 0/1 (project)") } func TestRenderListTextGroupsExperimental(t *testing.T) { @@ -361,7 +361,7 @@ func TestRenderListTextGroupsExperimental(t *testing.T) { renderListText(ctx, out, installer.ScopeGlobal) got := stderr.String() - availIdx := strings.Index(got, "Available skills") + availIdx := strings.Index(got, "Available raw skill directories") expIdx := strings.Index(got, "Experimental skills:") require.GreaterOrEqual(t, availIdx, 0, "available group header present") require.GreaterOrEqual(t, expIdx, 0, "experimental group header present") @@ -373,6 +373,38 @@ func TestRenderListTextGroupsExperimental(t *testing.T) { assert.NotContains(t, got, "[experimental]") } +func TestRenderListTextShowsPluginInstallsBeforeRawSkills(t *testing.T) { + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + out := listOutput{ + Release: "0.2.6", + Skills: []skillEntry{ + {Name: "databricks-jobs", LatestVersion: "1.0.0", Installed: map[string]string{}}, + }, + Summary: map[string]scopeSummary{ + installer.ScopeGlobal: {Installed: 0, Total: 1, loaded: true}, + }, + Agents: []agentEntry{ + { + Name: "claude-code", + Managed: true, + Installed: map[string]pluginInfo{installer.ScopeGlobal: {Version: "0.2.6", NativeScope: "user"}}, + }, + }, + } + + renderListText(ctx, out, installer.ScopeGlobal) + + got := stderr.String() + pluginIdx := strings.Index(got, "Plugin installs:") + rawIdx := strings.Index(got, "Available raw skill directories") + require.GreaterOrEqual(t, pluginIdx, 0) + require.GreaterOrEqual(t, rawIdx, 0) + assert.Less(t, pluginIdx, rawIdx) + assert.Contains(t, got, "Claude Code") + assert.Contains(t, got, "databricks plugin · v0.2.6 · up to date") + assert.Contains(t, got, "0/1 raw skill directories installed (global)") +} + func TestListScopeFlag(t *testing.T) { tests := []struct { name string diff --git a/cmd/aitools/uninstall.go b/cmd/aitools/uninstall.go index 2bf53f9ad9..3336f92e6d 100644 --- a/cmd/aitools/uninstall.go +++ b/cmd/aitools/uninstall.go @@ -16,7 +16,7 @@ var uninstallSkillsFn = func(ctx context.Context, opts installer.UninstallOption } func NewUninstallCmd() *cobra.Command { - var skillsFlag, scopeFlag string + var skillsFlag, agentsFlag, scopeFlag string var projectFlag, globalFlag, keepMarketplace bool cmd := &cobra.Command{ @@ -53,6 +53,16 @@ By default, removes all skills. Use --skills to remove specific skills only.`, KeepMarketplace: keepMarketplace, } opts.Skills = splitAndTrim(skillsFlag) + if agentsFlag != "" { + targetAgents, err := resolveAgentNames(ctx, agentsFlag) + if err != nil { + return err + } + opts.Agents = make([]string, 0, len(targetAgents)) + for _, agent := range targetAgents { + opts.Agents = append(opts.Agents, agent.Name) + } + } // Uninstall is destructive, so confirm interactively before doing // anything. Non-interactive runs (no TTY) proceed unprompted so @@ -77,6 +87,7 @@ By default, removes all skills. Use --skills to remove specific skills only.`, } cmd.Flags().StringVar(&skillsFlag, "skills", "", "Specific skills to uninstall (comma-separated)") + cmd.Flags().StringVar(&agentsFlag, "agents", "", "Agents to uninstall from (comma-separated, e.g. claude-code,cursor)") cmd.Flags().BoolVar(&keepMarketplace, "keep-marketplace", false, "Keep the marketplace registration when removing a plugin") cmd.Flags().StringVar(&scopeFlag, "scope", "", "Uninstall scope: project or global") cmd.Flags().BoolVar(&projectFlag, "project", false, "Uninstall project-scoped skills") @@ -108,8 +119,12 @@ func uninstallConfirmMessage(state *installer.InstallState, opts installer.Unins if state == nil { return "", false } + target := "all agents" + if len(opts.Agents) > 0 { + target = strings.Join(opts.Agents, ", ") + } if len(opts.Skills) > 0 { - return fmt.Sprintf("This will remove %s %s (%s scope).", plural(len(opts.Skills), "skill"), strings.Join(opts.Skills, ", "), opts.Scope), true + return fmt.Sprintf("This will remove %s %s from %s (%s scope).", plural(len(opts.Skills), "skill"), strings.Join(opts.Skills, ", "), target, opts.Scope), true } var parts []string if n := len(state.Skills); n > 0 { @@ -121,7 +136,7 @@ func uninstallConfirmMessage(state *installer.InstallState, opts installer.Unins if len(parts) == 0 { return "", false } - return fmt.Sprintf("This will remove %s (%s scope).", strings.Join(parts, " and "), opts.Scope), true + return fmt.Sprintf("This will remove %s from %s (%s scope).", strings.Join(parts, " and "), target, opts.Scope), true } // plural returns noun for n == 1 and noun+"s" otherwise. diff --git a/cmd/aitools/uninstall_test.go b/cmd/aitools/uninstall_test.go index f9a8016a82..04c70393c3 100644 --- a/cmd/aitools/uninstall_test.go +++ b/cmd/aitools/uninstall_test.go @@ -65,6 +65,37 @@ func TestUninstallScopeFlag(t *testing.T) { } } +func TestUninstallAgentsFlag(t *testing.T) { + setupTestAgents(t) + calls := setupUninstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := NewUninstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--scope", "global", "--agents", "cursor,claude-code", "--skills", "databricks-sql"}) + + require.NoError(t, cmd.Execute()) + require.Len(t, *calls, 1) + assert.Equal(t, []string{"cursor", "claude-code"}, (*calls)[0].Agents) + assert.Equal(t, []string{"databricks-sql"}, (*calls)[0].Skills) +} + +func TestUninstallUnknownAgentErrors(t *testing.T) { + setupTestAgents(t) + setupUninstallMock(t) + + ctx := cmdio.MockDiscard(t.Context()) + cmd := NewUninstallCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--scope", "global", "--agents", "invalid-agent"}) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown agent") +} + func TestUninstallConfirmMessage(t *testing.T) { // Nothing recorded: no prompt (installer surfaces its own guidance). _, ask := uninstallConfirmMessage(nil, installer.UninstallOptions{Scope: installer.ScopeGlobal}) @@ -90,5 +121,12 @@ func TestUninstallConfirmMessage(t *testing.T) { msg2, ask2 := uninstallConfirmMessage(st, filtered) require.True(t, ask2) assert.Contains(t, msg2, "skill alpha") + assert.Contains(t, msg2, "from all agents") assert.Contains(t, msg2, "(project scope)") + + targeted := installer.UninstallOptions{Scope: installer.ScopeGlobal, Agents: []string{"cursor"}} + targeted.Skills = []string{"alpha"} + msg3, ask3 := uninstallConfirmMessage(st, targeted) + require.True(t, ask3) + assert.Contains(t, msg3, "from cursor") } diff --git a/cmd/aitools/update.go b/cmd/aitools/update.go index a0d3be2896..0a521e9743 100644 --- a/cmd/aitools/update.go +++ b/cmd/aitools/update.go @@ -3,10 +3,13 @@ package aitools import ( "context" "fmt" + "maps" + "slices" "github.com/databricks/cli/libs/aitools/agents" "github.com/databricks/cli/libs/aitools/installer" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/log" "github.com/spf13/cobra" ) @@ -15,7 +18,23 @@ var updateSkillsFn = func(ctx context.Context, src installer.ManifestSource, ins return installer.UpdateSkills(ctx, src, installed, opts) } -var updatePluginsFn = installer.UpdateInstalledPlugins +var ( + updatePluginsFn = installer.UpdateInstalledPlugins + updateInstallPluginForAgentFn = installer.InstallPluginForAgent + updateRecordPluginInstallsFn = installer.RecordPluginInstalls + updateCleanupLegacyFn = installer.RemoveLegacyRawSkills + hasManagedRawSkillsForAgentFn = installer.HasManagedRawSkillsForAgent +) + +type pluginMigrationCandidate struct { + agent *agents.Agent + nativeScope string +} + +type pluginMigrationResult struct { + agent *agents.Agent + record installer.PluginRecord +} func NewUpdateCmd() *cobra.Command { var check, force, noNew, noPrune bool @@ -75,6 +94,18 @@ preview what would change without downloading.`, return err } + if check && refErr == nil && state != nil && len(state.Plugins) > 0 { + printPluginCheckResults(ctx, state, installer.DisplaySkillsVersion(ref)) + } + + migrationCandidates, err := pluginMigrationCandidatesForScope(ctx, scope, state) + if err != nil { + return err + } + if check { + printPluginMigrationCheckResults(ctx, migrationCandidates) + } + // Update plugin agents through their own CLI (check mode skips this). if !check && refErr == nil { pluginUpdates, err := updatePluginsFn(ctx, scope, ref) @@ -84,6 +115,17 @@ preview what would change without downloading.`, for _, pu := range pluginUpdates { cmdio.LogString(ctx, fmt.Sprintf(" %s databricks plugin %s", pu.Agent, versionToken(pu.Version))) } + + migrated, err := migrateLegacyRawSkillsToPlugins(ctx, scope, ref, migrationCandidates) + if err != nil { + return err + } + if migrated { + state, err = installer.LoadState(dir) + if err != nil { + return err + } + } } // Reconcile file skills for non-plugin agents. Skip entirely for a @@ -103,8 +145,11 @@ preview what would change without downloading.`, if err != nil { return err } - if result != nil && (len(result.Updated) > 0 || len(result.Added) > 0 || len(result.Removed) > 0) { - cmdio.LogString(ctx, installer.FormatUpdateResult(result, check)) + if result != nil && (check || len(result.Updated) > 0 || len(result.Added) > 0 || len(result.Removed) > 0) { + text := installer.FormatUpdateResult(result, check) + if text != "No changes." || len(migrationCandidates) == 0 { + cmdio.LogString(ctx, text) + } } } } @@ -124,6 +169,88 @@ preview what would change without downloading.`, return cmd } +func pluginMigrationCandidatesForScope(ctx context.Context, scope string, state *installer.InstallState) ([]pluginMigrationCandidate, error) { + var candidates []pluginMigrationCandidate + for _, agent := range agents.Registry { + if agent.Plugin == nil { + continue + } + if state != nil { + if _, alreadyPlugin := state.Plugins[agent.Name]; alreadyPlugin { + continue + } + } + nativeScope, ok, _ := mapAgentScope(agent, scope) + if !ok { + continue + } + // Match install's non-interactive behavior: a raw install for a plugin + // agent should attempt migration and report a skip if its CLI is missing, + // rather than silently staying on raw skills forever. + hasRawSkills, err := hasManagedRawSkillsForAgentFn(ctx, agent, scope) + if err != nil { + return nil, err + } + if !hasRawSkills { + continue + } + candidates = append(candidates, pluginMigrationCandidate{agent: agent, nativeScope: nativeScope}) + } + return candidates, nil +} + +func printPluginMigrationCheckResults(ctx context.Context, candidates []pluginMigrationCandidate) { + for _, candidate := range candidates { + cmdio.LogString(ctx, fmt.Sprintf(" %s would migrate raw skills to databricks plugin", candidate.agent.DisplayName)) + } +} + +func migrateLegacyRawSkillsToPlugins(ctx context.Context, scope, ref string, candidates []pluginMigrationCandidate) (bool, error) { + if len(candidates) == 0 { + return false, nil + } + + records := map[string]installer.PluginRecord{} + migrated := make([]pluginMigrationResult, 0, len(candidates)) + for _, candidate := range candidates { + agent := candidate.agent + cmdio.LogString(ctx, fmt.Sprintf("Installing databricks plugin for %s...", agent.DisplayName)) + rec, err := updateInstallPluginForAgentFn(ctx, agent, candidate.nativeScope, ref) + if err != nil { + cmdio.LogString(ctx, cmdio.Yellow(ctx, fmt.Sprintf("Skipped %s: %v", agent.DisplayName, err))) + continue + } + records[agent.Name] = rec + migrated = append(migrated, pluginMigrationResult{agent: agent, record: rec}) + } + if len(records) == 0 { + return false, nil + } + if err := updateRecordPluginInstallsFn(ctx, scope, records, ref); err != nil { + return false, err + } + for _, result := range migrated { + if err := updateCleanupLegacyFn(ctx, result.agent, scope); err != nil { + log.Debugf(ctx, "Legacy skill cleanup for %s failed: %v", result.agent.DisplayName, err) + } + cmdio.LogString(ctx, fmt.Sprintf(" %s databricks plugin %s", result.agent.DisplayName, versionToken(result.record.Version))) + } + return true, nil +} + +func printPluginCheckResults(ctx context.Context, state *installer.InstallState, latest string) { + for _, name := range slices.Sorted(maps.Keys(state.Plugins)) { + rec := state.Plugins[name] + status := "up to date" + if rec.Version == "" { + status = "version unknown" + } else if rec.Version != latest { + status = "update available" + } + cmdio.LogString(ctx, fmt.Sprintf(" %s databricks plugin %s (%s)", agentDisplayName(name), versionToken(rec.Version), status)) + } +} + // excludePluginAgents drops agents that are managed as plugins in this scope, so // the file-skills reconcile never drops duplicate skill files onto a plugin agent. func excludePluginAgents(installed []*agents.Agent, state *installer.InstallState) []*agents.Agent { diff --git a/cmd/aitools/update_test.go b/cmd/aitools/update_test.go index 5016fd9a18..2dee55bc5c 100644 --- a/cmd/aitools/update_test.go +++ b/cmd/aitools/update_test.go @@ -2,6 +2,7 @@ package aitools import ( "context" + "errors" "testing" "github.com/databricks/cli/libs/aitools/agents" @@ -24,6 +25,308 @@ func setupUpdateMock(t *testing.T) *[]installer.UpdateOptions { return &calls } +func resetUpdatePluginSeams(t *testing.T) { + t.Helper() + origUpdatePlugins := updatePluginsFn + origInstallPlugin := updateInstallPluginForAgentFn + origRecordPlugins := updateRecordPluginInstallsFn + origCleanupLegacy := updateCleanupLegacyFn + origHasManagedRawSkills := hasManagedRawSkillsForAgentFn + t.Cleanup(func() { + updatePluginsFn = origUpdatePlugins + updateInstallPluginForAgentFn = origInstallPlugin + updateRecordPluginInstallsFn = origRecordPlugins + updateCleanupLegacyFn = origCleanupLegacy + hasManagedRawSkillsForAgentFn = origHasManagedRawSkills + }) +} + +func TestUpdateCheckPrintsNoChanges(t *testing.T) { + setupTestAgents(t) + t.Setenv("DATABRICKS_SKILLS_REF", "v0.2.6") + setupUpdateMock(t) + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + cmd := NewUpdateCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--scope", "global", "--check"}) + + require.NoError(t, cmd.Execute()) + assert.Contains(t, stderr.String(), "No changes.") +} + +func TestUpdateCheckPrintsPluginState(t *testing.T) { + setupTestAgents(t) + t.Setenv("DATABRICKS_SKILLS_REF", "v0.2.6") + + origUpdateSkills := updateSkillsFn + origUpdatePlugins := updatePluginsFn + t.Cleanup(func() { + updateSkillsFn = origUpdateSkills + updatePluginsFn = origUpdatePlugins + }) + updateSkillsFn = func(context.Context, installer.ManifestSource, []*agents.Agent, installer.UpdateOptions) (*installer.UpdateResult, error) { + require.Fail(t, "pure plugin check should not update raw skills") + return nil, nil + } + updatePluginsFn = func(context.Context, string, string) ([]installer.PluginUpdate, error) { + require.Fail(t, "check mode must not run plugin update commands") + return nil, nil + } + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + dir, err := installer.GlobalSkillsDir(ctx) + require.NoError(t, err) + require.NoError(t, installer.SaveState(dir, &installer.InstallState{ + SchemaVersion: 2, + Release: "v0.2.6", + Plugins: map[string]installer.PluginRecord{ + agents.NameClaudeCode: {Marketplace: "claude-plugins-official", Plugin: "databricks", Version: "0.2.6"}, + }, + })) + + cmd := NewUpdateCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--scope", "global", "--check"}) + + require.NoError(t, cmd.Execute()) + assert.Contains(t, stderr.String(), "Claude Code databricks plugin v0.2.6 (up to date)") +} + +func TestUpdateMigratesManagedRawSkillsToPlugins(t *testing.T) { + setupTestAgents(t) + t.Setenv("DATABRICKS_SKILLS_REF", "v0.2.6") + resetUpdatePluginSeams(t) + + var rawUpdateAgents []string + origUpdateSkills := updateSkillsFn + t.Cleanup(func() { updateSkillsFn = origUpdateSkills }) + updateSkillsFn = func(_ context.Context, _ installer.ManifestSource, targetAgents []*agents.Agent, _ installer.UpdateOptions) (*installer.UpdateResult, error) { + for _, a := range targetAgents { + rawUpdateAgents = append(rawUpdateAgents, a.Name) + } + return &installer.UpdateResult{}, nil + } + updatePluginsFn = func(context.Context, string, string) ([]installer.PluginUpdate, error) { + return nil, nil + } + + var installed []pluginCall + updateInstallPluginForAgentFn = func(_ context.Context, a *agents.Agent, scope, ref string) (installer.PluginRecord, error) { + installed = append(installed, pluginCall{agent: a.Name, scope: scope}) + return installer.PluginRecord{ + Marketplace: a.Plugin.Marketplace, + Plugin: a.Plugin.ID, + Scope: scope, + Version: installer.DisplaySkillsVersion(ref), + }, nil + } + + var cleaned []string + updateCleanupLegacyFn = func(_ context.Context, a *agents.Agent, _ string) error { + cleaned = append(cleaned, a.Name) + return nil + } + updateRecordPluginInstallsFn = func(ctx context.Context, scope string, records map[string]installer.PluginRecord, ref string) error { + return installer.RecordPluginInstalls(ctx, scope, records, ref) + } + hasManagedRawSkillsForAgentFn = func(_ context.Context, a *agents.Agent, _ string) (bool, error) { + return a.Plugin != nil, nil + } + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + dir, err := installer.GlobalSkillsDir(ctx) + require.NoError(t, err) + require.NoError(t, installer.SaveState(dir, &installer.InstallState{ + SchemaVersion: 2, + Release: "v0.2.5", + Skills: map[string]string{"databricks": "0.2.5"}, + })) + + cmd := NewUpdateCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--scope", "global"}) + + require.NoError(t, cmd.Execute()) + + assert.Equal(t, []pluginCall{ + {agent: agents.NameClaudeCode, scope: agentScopeUser}, + {agent: agents.NameCodex, scope: agentScopeUser}, + {agent: agents.NameCopilot, scope: agentScopeUser}, + }, installed) + assert.Equal(t, []string{agents.NameClaudeCode, agents.NameCodex, agents.NameCopilot}, cleaned) + assert.Equal(t, []string{agents.NameCursor}, rawUpdateAgents) + assert.Contains(t, stderr.String(), "Installing databricks plugin for Claude Code...") + assert.Contains(t, stderr.String(), "GitHub Copilot databricks plugin v0.2.6") + + state, err := installer.LoadState(dir) + require.NoError(t, err) + require.NotNil(t, state) + assert.Contains(t, state.Plugins, agents.NameClaudeCode) + assert.Contains(t, state.Plugins, agents.NameCodex) + assert.Contains(t, state.Plugins, agents.NameCopilot) +} + +func TestUpdateKeepsRawSkillsWhenPluginMigrationStateWriteFails(t *testing.T) { + setupTestAgents(t) + t.Setenv("DATABRICKS_SKILLS_REF", "v0.2.6") + resetUpdatePluginSeams(t) + + origUpdateSkills := updateSkillsFn + t.Cleanup(func() { updateSkillsFn = origUpdateSkills }) + updateSkillsFn = func(context.Context, installer.ManifestSource, []*agents.Agent, installer.UpdateOptions) (*installer.UpdateResult, error) { + require.Fail(t, "raw skill update should not continue after migration state write fails") + return nil, nil + } + updatePluginsFn = func(context.Context, string, string) ([]installer.PluginUpdate, error) { + return nil, nil + } + + var installed []string + updateInstallPluginForAgentFn = func(_ context.Context, a *agents.Agent, scope, ref string) (installer.PluginRecord, error) { + installed = append(installed, a.Name) + return installer.PluginRecord{ + Marketplace: a.Plugin.Marketplace, + Plugin: a.Plugin.ID, + Scope: scope, + Version: installer.DisplaySkillsVersion(ref), + }, nil + } + + var cleaned []string + updateCleanupLegacyFn = func(_ context.Context, a *agents.Agent, _ string) error { + cleaned = append(cleaned, a.Name) + return nil + } + updateRecordPluginInstallsFn = func(context.Context, string, map[string]installer.PluginRecord, string) error { + return errors.New("state write failed") + } + hasManagedRawSkillsForAgentFn = func(_ context.Context, a *agents.Agent, _ string) (bool, error) { + return a.Plugin != nil, nil + } + + ctx := cmdio.MockDiscard(t.Context()) + dir, err := installer.GlobalSkillsDir(ctx) + require.NoError(t, err) + require.NoError(t, installer.SaveState(dir, &installer.InstallState{ + SchemaVersion: 2, + Release: "v0.2.5", + Skills: map[string]string{"databricks": "0.2.5"}, + })) + + cmd := NewUpdateCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--scope", "global"}) + + err = cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "state write failed") + assert.Equal(t, []string{agents.NameClaudeCode, agents.NameCodex, agents.NameCopilot}, installed) + assert.Empty(t, cleaned) + + state, err := installer.LoadState(dir) + require.NoError(t, err) + require.NotNil(t, state) + assert.Empty(t, state.Plugins) + assert.Equal(t, map[string]string{"databricks": "0.2.5"}, state.Skills) +} + +func TestUpdateCheckPrintsPluginMigrationPlan(t *testing.T) { + setupTestAgents(t) + t.Setenv("DATABRICKS_SKILLS_REF", "v0.2.6") + resetUpdatePluginSeams(t) + setupUpdateMock(t) + + updatePluginsFn = func(context.Context, string, string) ([]installer.PluginUpdate, error) { + require.Fail(t, "check mode must not run plugin update commands") + return nil, nil + } + updateInstallPluginForAgentFn = func(context.Context, *agents.Agent, string, string) (installer.PluginRecord, error) { + require.Fail(t, "check mode must not install plugins") + return installer.PluginRecord{}, nil + } + updateRecordPluginInstallsFn = func(context.Context, string, map[string]installer.PluginRecord, string) error { + require.Fail(t, "check mode must not record plugin installs") + return nil + } + updateCleanupLegacyFn = func(context.Context, *agents.Agent, string) error { + require.Fail(t, "check mode must not clean up legacy skills") + return nil + } + hasManagedRawSkillsForAgentFn = func(_ context.Context, a *agents.Agent, _ string) (bool, error) { + return a.Plugin != nil, nil + } + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + dir, err := installer.GlobalSkillsDir(ctx) + require.NoError(t, err) + require.NoError(t, installer.SaveState(dir, &installer.InstallState{ + SchemaVersion: 2, + Release: "v0.2.5", + Skills: map[string]string{"databricks": "0.2.5"}, + })) + + cmd := NewUpdateCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--scope", "global", "--check"}) + + require.NoError(t, cmd.Execute()) + assert.Contains(t, stderr.String(), "Claude Code would migrate raw skills to databricks plugin") + assert.Contains(t, stderr.String(), "Codex CLI would migrate raw skills to databricks plugin") + assert.Contains(t, stderr.String(), "GitHub Copilot would migrate raw skills to databricks plugin") + assert.NotContains(t, stderr.String(), "No changes.") +} + +func TestUpdateDoesNotMigratePluginAgentWithoutManagedRawSkills(t *testing.T) { + setupTestAgents(t) + t.Setenv("DATABRICKS_SKILLS_REF", "v0.2.6") + resetUpdatePluginSeams(t) + + var rawUpdateAgents []string + origUpdateSkills := updateSkillsFn + t.Cleanup(func() { updateSkillsFn = origUpdateSkills }) + updateSkillsFn = func(_ context.Context, _ installer.ManifestSource, targetAgents []*agents.Agent, _ installer.UpdateOptions) (*installer.UpdateResult, error) { + for _, a := range targetAgents { + rawUpdateAgents = append(rawUpdateAgents, a.Name) + } + return &installer.UpdateResult{}, nil + } + updatePluginsFn = func(context.Context, string, string) ([]installer.PluginUpdate, error) { + return nil, nil + } + updateInstallPluginForAgentFn = func(context.Context, *agents.Agent, string, string) (installer.PluginRecord, error) { + require.Fail(t, "must not migrate without managed raw skills") + return installer.PluginRecord{}, nil + } + updateRecordPluginInstallsFn = func(context.Context, string, map[string]installer.PluginRecord, string) error { + require.Fail(t, "must not record plugin installs without migration") + return nil + } + updateCleanupLegacyFn = func(context.Context, *agents.Agent, string) error { + require.Fail(t, "must not clean up without migration") + return nil + } + hasManagedRawSkillsForAgentFn = func(context.Context, *agents.Agent, string) (bool, error) { + return false, nil + } + + ctx := cmdio.MockDiscard(t.Context()) + dir, err := installer.GlobalSkillsDir(ctx) + require.NoError(t, err) + require.NoError(t, installer.SaveState(dir, &installer.InstallState{ + SchemaVersion: 2, + Release: "v0.2.5", + Skills: map[string]string{"databricks": "0.2.5"}, + })) + + cmd := NewUpdateCmd() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--scope", "global"}) + + require.NoError(t, cmd.Execute()) + assert.Equal(t, []string{agents.NameClaudeCode, agents.NameCursor}, rawUpdateAgents) +} + func TestUpdateNoPruneFlag(t *testing.T) { setupTestAgents(t) calls := setupUpdateMock(t) diff --git a/cmd/aitools/version.go b/cmd/aitools/version.go index 3244618a03..739174a186 100644 --- a/cmd/aitools/version.go +++ b/cmd/aitools/version.go @@ -11,6 +11,7 @@ import ( "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/log" "github.com/spf13/cobra" + "golang.org/x/mod/semver" ) func NewVersionCmd() *cobra.Command { @@ -81,15 +82,25 @@ func NewVersionCmd() *cobra.Command { func printPluginLines(ctx context.Context, state *installer.InstallState, scope string) { for _, name := range slices.Sorted(maps.Keys(state.Plugins)) { rec := state.Plugins[name] - cmdio.LogString(ctx, fmt.Sprintf(" Plugin (%s, %s): %s", agentDisplayName(name), scope, versionToken(rec.Version))) + scopeLabel := scope + if rec.Scope != "" { + scopeLabel += ", " + rec.Scope + " scope" + } + cmdio.LogString(ctx, fmt.Sprintf(" Plugin (%s, %s): %s", agentDisplayName(name), scopeLabel, versionToken(rec.Version))) } } -// versionToken renders a skills/plugin version for output: the "latest" sentinel -// is shown as-is; a concrete version gets a leading "v". +// versionToken renders a skills/plugin version for output: the "latest" +// sentinel is explicit about tracking main; a concrete version gets a leading "v". func versionToken(v string) string { + if v == "" { + return "version unknown" + } if v == "latest" { - return "latest" + return "latest (tracking main)" + } + if !semver.IsValid("v" + v) { + return v } return "v" + v } diff --git a/cmd/aitools/version_test.go b/cmd/aitools/version_test.go index c314153f8d..cf4a6516e0 100644 --- a/cmd/aitools/version_test.go +++ b/cmd/aitools/version_test.go @@ -25,7 +25,7 @@ func TestVersionShowsPlugin(t *testing.T) { Release: "v0.2.6", LastUpdated: time.Date(2026, 6, 24, 0, 0, 0, 0, time.UTC), Plugins: map[string]installer.PluginRecord{ - "claude-code": {Marketplace: "databricks-agent-skills", Plugin: "databricks", Version: "0.2.6"}, + "claude-code": {Marketplace: "databricks-agent-skills", Plugin: "databricks", Scope: "user", Version: "0.2.6"}, }, })) @@ -34,7 +34,35 @@ func TestVersionShowsPlugin(t *testing.T) { cmd.SetContext(ctx) require.NoError(t, cmd.RunE(cmd, nil)) - assert.Contains(t, stderr.String(), "Plugin (Claude Code, global): v0.2.6") + assert.Contains(t, stderr.String(), "Plugin (Claude Code, global, user scope): v0.2.6") +} + +func TestVersionClarifiesLatest(t *testing.T) { + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("USERPROFILE", tmp) + t.Setenv("DATABRICKS_SKILLS_REF", "main") + t.Chdir(tmp) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + require.NoError(t, installer.SaveState(globalDir, &installer.InstallState{ + SchemaVersion: 2, + Release: "main", + LastUpdated: time.Date(2026, 6, 24, 0, 0, 0, 0, time.UTC), + Skills: map[string]string{"databricks-sql": "0.1.0"}, + Plugins: map[string]installer.PluginRecord{ + "claude-code": {Marketplace: "claude-plugins-official", Plugin: "databricks", Scope: "user", Version: "latest"}, + }, + })) + + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + cmd := NewVersionCmd() + cmd.SetContext(ctx) + require.NoError(t, cmd.RunE(cmd, nil)) + + output := stderr.String() + assert.Contains(t, output, "Skills (global): main") + assert.Contains(t, output, "Plugin (Claude Code, global, user scope): latest (tracking main)") } func TestVersionShowsBothScopes(t *testing.T) { diff --git a/libs/aitools/agents/agents.go b/libs/aitools/agents/agents.go index 4f9a4d082b..664288d138 100644 --- a/libs/aitools/agents/agents.go +++ b/libs/aitools/agents/agents.go @@ -124,7 +124,7 @@ const ( ) // databricksPlugin returns the shared plugin descriptor for an agent that -// installs from our own marketplace (Codex, Copilot, Cursor). +// installs from our own marketplace (Codex, Copilot). func databricksPlugin() *PluginSpec { return &PluginSpec{ Marketplace: databricksMarketplace, @@ -157,11 +157,9 @@ var Registry = []*Agent{ Plugin: claudePlugin(), }, { - Name: NameCursor, - DisplayName: "Cursor", - ConfigDir: homeSubdir(".cursor"), - SupportsProjectScope: true, - ProjectConfigDir: ".cursor", + Name: NameCursor, + DisplayName: "Cursor", + ConfigDir: homeSubdir(".cursor"), // Cursor's CLI binary is `cursor-agent`, not `cursor` (the latter is an // IDE shim that isn't on PATH unless the user ran "install shell command"). Binary: "cursor-agent", diff --git a/libs/aitools/installer/cleanup.go b/libs/aitools/installer/cleanup.go index 36c9bb4880..a307ef8547 100644 --- a/libs/aitools/installer/cleanup.go +++ b/libs/aitools/installer/cleanup.go @@ -18,48 +18,71 @@ import ( // the recorded checksums. User-modified or third-party directories, dirs with // extra files, and copies with no recorded provenance are left untouched. func RemoveLegacyRawSkills(ctx context.Context, agent *agents.Agent, scope string) error { - baseDir, err := skillsDir(ctx, scope) + entries, err := managedRawSkillEntriesForAgent(ctx, agent, scope) if err != nil { return err } + + for _, entryPath := range entries { + // RemoveAll on a symlink removes the link, not its target. + if err := os.RemoveAll(entryPath); err != nil { + log.Warnf(ctx, "Failed to remove legacy skill %s: %v", entryPath, err) + } else { + log.Debugf(ctx, "Removed legacy skill %s for %s", filepath.Base(entryPath), agent.DisplayName) + } + } + return nil +} + +// HasManagedRawSkillsForAgent reports whether an agent has raw skill exposure +// that this CLI can safely remove after migrating that agent to the plugin. +func HasManagedRawSkillsForAgent(ctx context.Context, agent *agents.Agent, scope string) (bool, error) { + entries, err := managedRawSkillEntriesForAgent(ctx, agent, scope) + if err != nil { + return false, err + } + return len(entries) > 0, nil +} + +func managedRawSkillEntriesForAgent(ctx context.Context, agent *agents.Agent, scope string) ([]string, error) { + baseDir, err := skillsDir(ctx, scope) + if err != nil { + return nil, err + } state, err := LoadState(baseDir) if err != nil { - return err + return nil, err } var cwd string if scope == ScopeProject { cwd, err = os.Getwd() if err != nil { - return err + return nil, err } } agentDir, err := agentSkillsDirForScope(ctx, agent, scope, cwd) if err != nil { - return err + return nil, err } entries, err := os.ReadDir(agentDir) if errors.Is(err, fs.ErrNotExist) { // No skills dir for this agent; nothing to clean up. - return nil + return nil, nil } if err != nil { - return err + return nil, err } + var managed []string for _, entry := range entries { entryPath := filepath.Join(agentDir, entry.Name()) canonicalDir := filepath.Join(baseDir, entry.Name()) if !exposureRemovable(entryPath, canonicalDir, entry.Name(), state) { continue } - // RemoveAll on a symlink removes the link, not its target. - if err := os.RemoveAll(entryPath); err != nil { - log.Warnf(ctx, "Failed to remove legacy skill %s: %v", entryPath, err) - } else { - log.Debugf(ctx, "Removed legacy skill %s for %s", entry.Name(), agent.DisplayName) - } + managed = append(managed, entryPath) } - return nil + return managed, nil } diff --git a/libs/aitools/installer/cleanup_test.go b/libs/aitools/installer/cleanup_test.go index 27cfc716c4..b19d627265 100644 --- a/libs/aitools/installer/cleanup_test.go +++ b/libs/aitools/installer/cleanup_test.go @@ -75,6 +75,46 @@ func TestRemoveLegacyRawSkills(t *testing.T) { assertExists(t, filepath.Join(agentDir, "delta")) } +func TestHasManagedRawSkillsForAgent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + ctx := cmdio.MockDiscard(t.Context()) + + baseDir, err := GlobalSkillsDir(ctx) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Join(baseDir, "alpha"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(baseDir, "alpha", "SKILL.md"), []byte("alpha"), 0o644)) + + agentDir := filepath.Join(home, ".claude", "skills") + require.NoError(t, os.MkdirAll(agentDir, 0o755)) + require.NoError(t, os.Symlink(filepath.Join(baseDir, "alpha"), filepath.Join(agentDir, "alpha"))) + require.NoError(t, os.MkdirAll(filepath.Join(agentDir, "thirdparty"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(agentDir, "thirdparty", "SKILL.md"), []byte("mine"), 0o644)) + + require.NoError(t, SaveState(baseDir, &InstallState{ + SchemaVersion: schemaVersionV2, + Files: map[string]FileRecord{ + "alpha/SKILL.md": {SHA256: sha("alpha")}, + }, + })) + + agent := &agents.Agent{ + Name: agents.NameClaudeCode, + DisplayName: "Claude Code", + ConfigDir: func(_ context.Context) (string, error) { return filepath.Join(home, ".claude"), nil }, + } + + has, err := HasManagedRawSkillsForAgent(ctx, agent, ScopeGlobal) + require.NoError(t, err) + assert.True(t, has) + + require.NoError(t, RemoveLegacyRawSkills(ctx, agent, ScopeGlobal)) + has, err = HasManagedRawSkillsForAgent(ctx, agent, ScopeGlobal) + require.NoError(t, err) + assert.False(t, has) +} + func assertGone(t *testing.T, path string) { t.Helper() _, err := os.Lstat(path) diff --git a/libs/aitools/installer/installer_test.go b/libs/aitools/installer/installer_test.go index f32f737dc0..13c870cbeb 100644 --- a/libs/aitools/installer/installer_test.go +++ b/libs/aitools/installer/installer_test.go @@ -1039,7 +1039,7 @@ func TestInstallKeepsNameWhenRepoDirChanges(t *testing.T) { func TestSupportsProjectScopeSetCorrectly(t *testing.T) { expected := map[string]bool{ "claude-code": true, - "cursor": true, + "cursor": false, "codex": false, "opencode": false, "copilot": false, diff --git a/libs/aitools/installer/plugin_test.go b/libs/aitools/installer/plugin_test.go index 15415a6106..2652918c44 100644 --- a/libs/aitools/installer/plugin_test.go +++ b/libs/aitools/installer/plugin_test.go @@ -276,6 +276,39 @@ func TestUninstallSkillsOptsTearsDownPlugin(t *testing.T) { assert.Nil(t, state) } +func TestUninstallSkillsOptsTargetsPluginAgents(t *testing.T) { + setupTestHome(t) + stubAgentLookPath(t, true) + ctx, stub := process.WithStub(t.Context()) + stub.WithCallback(func(*exec.Cmd) error { return nil }) + ctx = cmdio.MockDiscard(ctx) + + dir, err := GlobalSkillsDir(ctx) + require.NoError(t, err) + require.NoError(t, SaveState(dir, &InstallState{ + SchemaVersion: schemaVersionV2, + Release: "v0.2.6", + Plugins: map[string]PluginRecord{ + agents.NameClaudeCode: {Marketplace: "claude-plugins-official", Plugin: "databricks", Scope: "user"}, + agents.NameCopilot: {Marketplace: "databricks-agent-skills", Plugin: "databricks", Scope: "user"}, + }, + })) + + require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{Scope: ScopeGlobal, Agents: []string{agents.NameClaudeCode}})) + + cmds := stub.Commands() + assert.Contains(t, cmds, "claude plugin uninstall databricks@claude-plugins-official --scope user") + for _, cmd := range cmds { + assert.NotContains(t, cmd, "copilot plugin uninstall") + } + + state, err := LoadState(dir) + require.NoError(t, err) + require.NotNil(t, state) + assert.NotContains(t, state.Plugins, agents.NameClaudeCode) + assert.Contains(t, state.Plugins, agents.NameCopilot) +} + func TestUninstallNeverDeregistersBuiltinMarketplace(t *testing.T) { setupTestHome(t) stubAgentLookPath(t, true) diff --git a/libs/aitools/installer/uninstall.go b/libs/aitools/installer/uninstall.go index 2797373046..8207947499 100644 --- a/libs/aitools/installer/uninstall.go +++ b/libs/aitools/installer/uninstall.go @@ -19,6 +19,7 @@ import ( // UninstallOptions controls the behavior of UninstallSkillsOpts. type UninstallOptions struct { Skills []string // empty = all + Agents []string // empty = all agents Scope string // ScopeGlobal or ScopeProject (default: global) KeepMarketplace bool // keep the marketplace registration when removing a plugin } @@ -64,10 +65,15 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { // A full uninstall (no --skills filter) also tears down plugins. fullUninstall := len(opts.Skills) == 0 + agentFilter := stringSet(opts.Agents) + targetedAgents := len(agentFilter) > 0 pluginCount := 0 if fullUninstall { for _, name := range slices.Sorted(maps.Keys(state.Plugins)) { + if targetedAgents && !agentFilter[name] { + continue + } agent := agents.ByName(name) if agent == nil { // We can't tear down an agent we don't know; keep the record (and @@ -96,6 +102,11 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { seen := make(map[string]bool) for _, name := range opts.Skills { if _, ok := state.Skills[name]; !ok { + if targetedAgents { + if foundIn, found := untrackedSkillAgents(ctx, name, opts.Agents, scope, cwd); found { + return fmt.Errorf("skill %q is not tracked in Databricks aitools state but exists for %s. Run 'databricks aitools install --skills-only --agents %s' to rebuild state before uninstalling, or remove it manually", name, strings.Join(foundIn, ", "), strings.Join(opts.Agents, ",")) + } + } return fmt.Errorf("skill %q is not installed", name) } if seen[name] { @@ -113,7 +124,16 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { // Remove skill directories and symlinks for each skill. for _, name := range toRemove { canonicalDir := filepath.Join(baseDir, name) - removeSymlinksFromAgents(ctx, name, canonicalDir, scope, cwd) + removeCanonical := true + if targetedAgents { + removeSkillExposuresFromAgents(ctx, name, canonicalDir, scope, cwd, state, opts.Agents) + removeCanonical = !hasRemainingSkillExposure(ctx, name, canonicalDir, scope, cwd, opts.Agents) + } else { + removeSymlinksFromAgents(ctx, name, canonicalDir, scope, cwd) + } + if !removeCanonical { + continue + } if err := os.RemoveAll(canonicalDir); err != nil { log.Warnf(ctx, "Failed to remove %s: %v", canonicalDir, err) } @@ -157,6 +177,105 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { return nil } +func stringSet(items []string) map[string]bool { + if len(items) == 0 { + return nil + } + set := make(map[string]bool, len(items)) + for _, item := range items { + set[item] = true + } + return set +} + +func removeSkillExposuresFromAgents(ctx context.Context, skillName, canonicalDir, scope, cwd string, state *InstallState, agentNames []string) { + for _, name := range agentNames { + agent := agents.ByName(name) + if agent == nil { + continue + } + if scope == ScopeProject && !agent.SupportsProjectScope { + continue + } + agentDir, err := agentSkillsDirForScope(ctx, agent, scope, cwd) + if err != nil { + continue + } + entry := filepath.Join(agentDir, skillName) + if !exposureRemovable(entry, canonicalDir, skillName, state) { + log.Debugf(ctx, "Skipping unmanaged skill %s for %s", entry, agent.DisplayName) + continue + } + if err := os.RemoveAll(entry); err != nil { + log.Warnf(ctx, "Failed to remove %s: %v", entry, err) + } + } +} + +func hasRemainingSkillExposure(ctx context.Context, skillName, canonicalDir, scope, cwd string, removedAgents []string) bool { + removed := stringSet(removedAgents) + for _, agent := range agents.Registry { + if removed[agent.Name] { + continue + } + if scope == ScopeProject && !agent.SupportsProjectScope { + continue + } + agentDir, err := agentSkillsDirForScope(ctx, agent, scope, cwd) + if err != nil { + continue + } + entry := filepath.Join(agentDir, skillName) + if skillExposureExists(entry, canonicalDir) { + return true + } + } + return false +} + +func skillExposureExists(entry, canonicalDir string) bool { + fi, err := os.Lstat(entry) + if errors.Is(err, fs.ErrNotExist) { + return false + } + if err != nil { + return true + } + if fi.Mode()&os.ModeSymlink == 0 { + return true + } + target, err := os.Readlink(entry) + if err != nil { + return true + } + abs := target + if !filepath.IsAbs(target) { + abs = filepath.Clean(filepath.Join(filepath.Dir(entry), target)) + } + return abs == canonicalDir || strings.HasPrefix(abs, canonicalDir+string(os.PathSeparator)) +} + +func untrackedSkillAgents(ctx context.Context, skillName string, agentNames []string, scope, cwd string) ([]string, bool) { + var found []string + for _, name := range agentNames { + agent := agents.ByName(name) + if agent == nil { + continue + } + if scope == ScopeProject && !agent.SupportsProjectScope { + continue + } + agentDir, err := agentSkillsDirForScope(ctx, agent, scope, cwd) + if err != nil { + continue + } + if _, err := os.Lstat(filepath.Join(agentDir, skillName)); err == nil { + found = append(found, agent.DisplayName) + } + } + return found, len(found) > 0 +} + // 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. diff --git a/libs/aitools/installer/uninstall_test.go b/libs/aitools/installer/uninstall_test.go index 7d483fdb4b..3bd7bfcc19 100644 --- a/libs/aitools/installer/uninstall_test.go +++ b/libs/aitools/installer/uninstall_test.go @@ -334,3 +334,76 @@ func TestUninstallSelectiveAllRemovesStateFile(t *testing.T) { _, err = os.Stat(filepath.Join(globalDir, ".state.json")) assert.ErrorIs(t, err, fs.ErrNotExist) } + +func TestUninstallTargetedAgentKeepsRemainingRawExposure(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".cursor"), 0o755)) + + claudeAgent := agents.ByName(agents.NameClaudeCode) + cursorAgent := agents.ByName(agents.NameCursor) + src := &mockManifestSource{manifest: testManifest()} + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{claudeAgent, cursorAgent}, InstallOptions{})) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + cursorLink := filepath.Join(tmp, ".cursor", "skills", "databricks-sql") + claudeLink := filepath.Join(tmp, ".claude", "skills", "databricks-sql") + require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{Agents: []string{agents.NameCursor}, Skills: []string{"databricks-sql"}})) + + _, err := os.Lstat(cursorLink) + assert.ErrorIs(t, err, fs.ErrNotExist) + _, err = os.Lstat(claudeLink) + assert.NoError(t, err) + _, err = os.Stat(filepath.Join(globalDir, "databricks-sql")) + assert.NoError(t, err) + + state, err := LoadState(globalDir) + require.NoError(t, err) + require.NotNil(t, state) + assert.Contains(t, state.Skills, "databricks-sql") +} + +func TestUninstallTargetedAgentRemovesCanonicalWhenNoExposureRemains(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".cursor"), 0o755)) + + cursorAgent := agents.ByName(agents.NameCursor) + src := &mockManifestSource{manifest: testManifest()} + require.NoError(t, InstallSkillsForAgents(ctx, src, []*agents.Agent{cursorAgent}, InstallOptions{SpecificSkills: []string{"databricks-sql"}})) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + cursorDir := filepath.Join(tmp, ".cursor", "skills", "databricks-sql") + require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{Agents: []string{agents.NameCursor}, Skills: []string{"databricks-sql"}})) + + _, err := os.Stat(cursorDir) + assert.ErrorIs(t, err, fs.ErrNotExist) + _, err = os.Stat(filepath.Join(globalDir, "databricks-sql")) + assert.ErrorIs(t, err, fs.ErrNotExist) + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.Nil(t, state) +} + +func TestUninstallTargetedUntrackedSkillGivesRecoveryGuidance(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".cursor", "skills", "databricks-sql"), 0o755)) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + require.NoError(t, SaveState(globalDir, &InstallState{ + SchemaVersion: schemaVersionV2, + Skills: map[string]string{"databricks-jobs": "0.1.0"}, + })) + + err := UninstallSkillsOpts(ctx, UninstallOptions{Agents: []string{agents.NameCursor}, Skills: []string{"databricks-sql"}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "not tracked in Databricks aitools state") + assert.Contains(t, err.Error(), "Cursor") + assert.Contains(t, err.Error(), "install --skills-only --agents cursor") +}