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