chore: fixup

Author: MarioCadenas

experimental/aitools/lib/installer/installer.go

@@ -27,6 +27,7 @@ const (
 	skillsRepoName       = "databricks-agent-skills"
 	skillsRepoPath       = "skills"
 	defaultSkillsRepoRef = "v0.1.4"
+	httpTimeout          = 30 * time.Second
 )
 
 // managedSkillMarker is written at the root of each materialized (non-symlink)
@@ -94,14 +95,17 @@ func skillFileRawURL(ref, skillName, filePath string) string {
 }
 
 func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byte, error) {
+	if rootPath, ok := strings.CutPrefix(filePath, sharedFilePrefix); ok && rootPath == "" {
+		return nil, fmt.Errorf("invalid manifest file entry: empty path after %q", sharedFilePrefix)
+	}
 	url := skillFileRawURL(ref, skillName, filePath)
 
 	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create request: %w", err)
 	}
 
-	client := &http.Client{Timeout: 30 * time.Second}
+	client := &http.Client{Timeout: httpTimeout}
 	resp, err := client.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("failed to fetch %s: %w", filePath, err)
@@ -147,6 +151,8 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent
 		if len(targetAgents) == 0 {
 			return fmt.Errorf("no agents support project-scoped skills. The following detected agents are global-only: %s", strings.Join(incompatible, ", "))
 		}
+	} else if len(targetAgents) == 0 {
+		return errors.New("no target agents")
 	}
 
 	// Load existing state for idempotency checks.
@@ -419,6 +425,7 @@ func installSkillForAgents(ctx context.Context, skillName string, files []string
 	// Symlinks point at the canonical tree (full manifest layout for every agent).
 	symlinkEligible := params.scope == ScopeProject || len(detectedAgents) > 1
 
+	installedForAgent := 0
 	for _, agent := range detectedAgents {
 		agentSkillDir, err := agentSkillsDirForScope(ctx, agent, params.scope, params.cwd)
 		if err != nil {
@@ -449,15 +456,22 @@ func installSkillForAgents(ctx context.Context, skillName string, files []string
 					log.Warnf(ctx, "Failed to install for %s: %v", agent.DisplayName, err)
 					continue
 				}
+				log.Debugf(ctx, "Installed %q for %s (materialized copy)", skillName, agent.DisplayName)
+			} else {
+				log.Debugf(ctx, "Installed %q for %s (symlinked)", skillName, agent.DisplayName)
 			}
-			log.Debugf(ctx, "Installed %q for %s (symlinked)", skillName, agent.DisplayName)
 		} else {
 			if err := copySkillMaterialized(canonicalDir, destDir); err != nil {
 				log.Warnf(ctx, "Failed to install for %s: %v", agent.DisplayName, err)
 				continue
 			}
 			log.Debugf(ctx, "Installed %q for %s", skillName, agent.DisplayName)
 		}
+		installedForAgent++
+	}
+
+	if len(detectedAgents) > 0 && installedForAgent == 0 {
+		return fmt.Errorf("failed to install %q for any agent", skillName)
 	}
 
 	return nil
@@ -486,19 +500,16 @@ func backupThirdPartySkill(ctx context.Context, destDir, canonicalDir, skillName
 	if fi.Mode()&os.ModeSymlink != 0 {
 		target, err := os.Readlink(destDir)
 		if err == nil {
-			absTarget := target
-			if !filepath.IsAbs(target) {
-				absTarget = filepath.Clean(filepath.Join(filepath.Dir(destDir), target))
-			}
-			if absTarget == canonicalDir {
+			if pathWithinOrEqual(resolveSymlinkTarget(destDir, target), canonicalDir) {
 				return nil
 			}
 		}
 	}
 
 	// Prior materialized install from this CLI; copySkillMaterialized will replace it.
 	if fi.IsDir() {
-		if _, err := os.Stat(filepath.Join(destDir, managedSkillMarker)); err == nil {
+		mi, err := os.Lstat(filepath.Join(destDir, managedSkillMarker))
+		if err == nil && mi.Mode().IsRegular() {
 			return nil
 		}
 	}
@@ -527,21 +538,20 @@ func installSkillToDir(ctx context.Context, ref, skillName, destDir string, file
 		return fmt.Errorf("failed to create directory: %w", err)
 	}
 
-	for _, file := range files {
-		content, err := fetchFileFn(ctx, ref, skillName, file)
-		if err != nil {
-			return err
-		}
-
-		// Strip the @root: prefix so shared assets land at a local path
-		// (e.g. "@root:assets/databricks.svg" → "assets/databricks.svg").
-		if rootPath, ok := strings.CutPrefix(file, sharedFilePrefix); ok {
+	for _, manifestFile := range files {
+		relForDisk := manifestFile
+		if rootPath, ok := strings.CutPrefix(manifestFile, sharedFilePrefix); ok {
 			if rootPath == "" {
 				return fmt.Errorf("invalid manifest file entry: empty path after %q", sharedFilePrefix)
 			}
-			file = rootPath
+			relForDisk = rootPath
+		}
+		destPath, err := safeSkillDestPath(destDir, relForDisk)
+		if err != nil {
+			return err
 		}
-		destPath, err := safeSkillDestPath(destDir, file)
+
+		content, err := fetchFileFn(ctx, ref, skillName, manifestFile)
 		if err != nil {
 			return err
 		}
@@ -550,9 +560,9 @@ func installSkillToDir(ctx context.Context, ref, skillName, destDir string, file
 			return fmt.Errorf("failed to create directory: %w", err)
 		}
 
-		log.Debugf(ctx, "Downloading %s/%s", skillName, file)
+		log.Debugf(ctx, "Downloading %s/%s", skillName, manifestFile)
 		if err := os.WriteFile(destPath, content, 0o644); err != nil {
-			return fmt.Errorf("failed to write %s: %w", file, err)
+			return fmt.Errorf("failed to write %s: %w", relForDisk, err)
 		}
 	}
 
@@ -580,6 +590,29 @@ func safeSkillDestPath(destDir, file string) (string, error) {
 	return destPath, nil
 }
 
+// resolveSymlinkTarget returns the absolute, cleaned path that a symlink at
+// linkPath points to, given the raw target from os.Readlink.
+func resolveSymlinkTarget(linkPath, target string) string {
+	if !filepath.IsAbs(target) {
+		return filepath.Clean(filepath.Join(filepath.Dir(linkPath), target))
+	}
+	return filepath.Clean(target)
+}
+
+// pathWithinOrEqual reports whether target is exactly base or lexically under base.
+func pathWithinOrEqual(target, base string) bool {
+	t := filepath.Clean(target)
+	b := filepath.Clean(base)
+	if t == b {
+		return true
+	}
+	rel, err := filepath.Rel(b, t)
+	if err != nil {
+		return false
+	}
+	return filepath.IsLocal(rel)
+}
+
 // copySkillMaterialized copies the full skill tree from src to dest and writes
 // managedSkillMarker so uninstall can remove the directory safely.
 func copySkillMaterialized(src, dest string) error {
@@ -599,6 +632,10 @@ func copySkillMaterialized(src, dest string) error {
 
 		target := filepath.Join(dest, rel)
 
+		if d.Type()&fs.ModeSymlink != 0 {
+			return fmt.Errorf("unexpected symlink in skill tree: %s", rel)
+		}
+
 		if d.IsDir() {
 			return os.MkdirAll(target, 0o755)
 		}
@@ -613,9 +650,14 @@ func copySkillMaterialized(src, dest string) error {
 		return os.WriteFile(target, data, 0o644)
 	})
 	if err != nil {
+		_ = os.RemoveAll(dest)
 		return err
 	}
-	return os.WriteFile(filepath.Join(dest, managedSkillMarker), nil, 0o644)
+	if werr := os.WriteFile(filepath.Join(dest, managedSkillMarker), nil, 0o644); werr != nil {
+		_ = os.RemoveAll(dest)
+		return fmt.Errorf("failed to write managed skill marker: %w", werr)
+	}
+	return nil
 }
 
 func createSymlink(source, dest string) error {

experimental/aitools/lib/installer/installer_test.go

@@ -31,6 +31,15 @@ func (m *mockManifestSource) FetchManifest(_ context.Context, _ string) (*Manife
 	return m.manifest, nil
 }
 
+func TestInstallSkillsForAgentsRejectsNoTargetAgents(t *testing.T) {
+	setupTestHome(t)
+	ctx := cmdio.MockDiscard(t.Context())
+	src := &mockManifestSource{manifest: testManifest()}
+	err := InstallSkillsForAgents(ctx, src, nil, InstallOptions{})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "no target agents")
+}
+
 func testManifest() *Manifest {
 	return &Manifest{
 		Version:   "1",

experimental/aitools/lib/installer/skillurl_test.go

@@ -46,3 +46,14 @@ func TestSafeSkillDestPath(t *testing.T) {
 	_, err = safeSkillDestPath(destDir, "foo/..")
 	require.Error(t, err)
 }
+
+func TestPathWithinOrEqual(t *testing.T) {
+	tmp := t.TempDir()
+	base := filepath.Join(tmp, "canonical")
+	sub := filepath.Join(base, "nested", "file")
+
+	assert.True(t, pathWithinOrEqual(base, base))
+	assert.True(t, pathWithinOrEqual(sub, base))
+	assert.False(t, pathWithinOrEqual(filepath.Join(tmp, "outside"), base))
+	assert.False(t, pathWithinOrEqual(filepath.Join(base, "..", "outside"), base))
+}

experimental/aitools/lib/installer/uninstall.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"github.com/databricks/cli/experimental/aitools/lib/agents"
 	"github.com/databricks/cli/libs/cmdio"
@@ -143,7 +142,8 @@ func removeSymlinksFromAgents(ctx context.Context, skillName, canonicalDir, scop
 				log.Debugf(ctx, "Skipping non-symlink %s for %s", destDir, agent.DisplayName)
 				continue
 			}
-			if _, err := os.Stat(filepath.Join(destDir, managedSkillMarker)); err != nil {
+			mi, err := os.Lstat(filepath.Join(destDir, managedSkillMarker))
+			if err != nil || !mi.Mode().IsRegular() {
 				log.Debugf(ctx, "Skipping directory without managed marker %s for %s", destDir, agent.DisplayName)
 				continue
 			}
@@ -161,15 +161,8 @@ func removeSymlinksFromAgents(ctx context.Context, skillName, canonicalDir, scop
 			continue
 		}
 
-		// Resolve relative symlinks to absolute for comparison.
-		absTarget := target
-		if !filepath.IsAbs(target) {
-			absTarget = filepath.Join(filepath.Dir(destDir), target)
-			absTarget = filepath.Clean(absTarget)
-		}
-
-		// Only remove if the symlink points into our canonical dir.
-		if !strings.HasPrefix(absTarget, canonicalDir+string(os.PathSeparator)) && absTarget != canonicalDir {
+		absTarget := resolveSymlinkTarget(destDir, target)
+		if !pathWithinOrEqual(absTarget, canonicalDir) {
 			log.Debugf(ctx, "Skipping symlink %s (points to %s, not %s)", destDir, absTarget, canonicalDir)
 			continue
 		}
@@ -217,14 +210,7 @@ func cleanOrphanedSymlinks(ctx context.Context, baseDir, scope, cwd string) {
 				continue
 			}
 
-			// Resolve relative symlinks to absolute for comparison.
-			absTarget := target
-			if !filepath.IsAbs(target) {
-				absTarget = filepath.Clean(filepath.Join(filepath.Dir(entryPath), target))
-			}
-
-			// Check if the symlink points into our managed skills dir.
-			if !strings.HasPrefix(absTarget, baseDir+string(os.PathSeparator)) && absTarget != baseDir {
+			if !pathWithinOrEqual(resolveSymlinkTarget(entryPath, target), baseDir) {
 				continue
 			}
 

experimental/aitools/lib/installer/uninstall_test.go

@@ -225,6 +225,37 @@ func TestUninstallLeavesNonCanonicalSymlinks(t *testing.T) {
 	assert.Contains(t, stderr.String(), "Uninstalled 1 skill.")
 }
 
+func TestUninstallRemovesManagedMaterializedCopies(t *testing.T) {
+	tmp := setupTestHome(t)
+	globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills")
+
+	state := &InstallState{
+		SchemaVersion: 1,
+		Release:       "v0.1.0",
+		LastUpdated:   time.Now(),
+		Skills: map[string]string{
+			"databricks-sql": "0.1.0",
+		},
+	}
+	require.NoError(t, SaveState(globalDir, state))
+	require.NoError(t, os.MkdirAll(filepath.Join(globalDir, "databricks-sql"), 0o755))
+
+	require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755))
+	agentSkillsDir := filepath.Join(tmp, ".claude", "skills")
+	managedDir := filepath.Join(agentSkillsDir, "databricks-sql")
+	require.NoError(t, os.MkdirAll(managedDir, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(managedDir, managedSkillMarker), nil, 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(managedDir, "SKILL.md"), []byte("managed"), 0o644))
+
+	ctx, stderr := cmdio.NewTestContextWithStderr(t.Context())
+	err := UninstallSkills(ctx)
+	require.NoError(t, err)
+
+	_, err = os.Lstat(managedDir)
+	assert.True(t, os.IsNotExist(err), "managed materialized skill dir should be removed")
+	assert.Contains(t, stderr.String(), "Uninstalled 1 skill.")
+}
+
 func TestUninstallLeavesNonSymlinkDirectories(t *testing.T) {
 	tmp := setupTestHome(t)
 	globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills")