From e5d060884fd9f3fdc4d6ed537713bda8ed66ea60 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 30 Jun 2026 11:52:29 +0200 Subject: [PATCH 1/2] 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/2] 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})