chore: fixup

Author: MarioCadenas

experimental/aitools/lib/agents/agents.go

@@ -23,14 +23,6 @@ type Agent struct {
 	// ProjectConfigDir is the config directory name relative to a project root
 	// (e.g., ".claude"). Only used when SupportsProjectScope is true.
 	ProjectConfigDir string
-	// PreferSkillCopy when true, installs each skill as a full directory copy
-	// instead of a symlink. Codex uses this so agents/openai.yaml and assets/
-	// resolve reliably under its config dir.
-	PreferSkillCopy bool
-	// InstallAgentMetadata when true, includes agent-specific UI metadata
-	// (agents/openai.yaml, assets/) during installation. Agents that don't
-	// consume these files skip them to keep their skills dirs clean.
-	InstallAgentMetadata bool
 }
 
 // Detected returns true if the agent is installed on the system.
@@ -95,11 +87,9 @@ var Registry = []Agent{
 		ProjectConfigDir:     ".cursor",
 	},
 	{
-		Name:                 "codex",
-		DisplayName:          "Codex CLI",
-		ConfigDir:            homeSubdir(".codex"),
-		PreferSkillCopy:      true,
-		InstallAgentMetadata: true,
+		Name:        "codex",
+		DisplayName: "Codex CLI",
+		ConfigDir:   homeSubdir(".codex"),
 	},
 	{
 		Name:        "opencode",

experimental/aitools/lib/installer/installer.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -28,6 +29,10 @@ const (
 	defaultSkillsRepoRef = "v0.1.4"
 )
 
+// managedSkillMarker is written at the root of each materialized (non-symlink)
+// skill install under an agent directory so uninstall can remove it safely.
+const managedSkillMarker = ".databricks-skill-managed"
+
 // fetchFileFn is the function used to download individual skill files.
 // It is a package-level var so tests can replace it with a mock.
 var fetchFileFn = fetchSkillFile
@@ -78,15 +83,18 @@ func FetchManifest(ctx context.Context) (*Manifest, error) {
 // to disk and adjusts the download URL accordingly.
 const sharedFilePrefix = "@root:"
 
-func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byte, error) {
-	var url string
+// skillFileRawURL returns the raw.githubusercontent.com URL for a manifest file entry.
+func skillFileRawURL(ref, skillName, filePath string) string {
 	if rootPath, ok := strings.CutPrefix(filePath, sharedFilePrefix); ok {
-		url = fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s",
+		return fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s",
 			skillsRepoOwner, skillsRepoName, ref, rootPath)
-	} else {
-		url = fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s/%s/%s",
-			skillsRepoOwner, skillsRepoName, ref, skillsRepoPath, skillName, filePath)
 	}
+	return fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s/%s/%s",
+		skillsRepoOwner, skillsRepoName, ref, skillsRepoPath, skillName, filePath)
+}
+
+func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byte, error) {
+	url := skillFileRawURL(ref, skillName, filePath)
 
 	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
 	if err != nil {
@@ -401,32 +409,14 @@ type installParams struct {
 	ref     string
 }
 
-// agentMetadataDirs lists subdirectory prefixes that are agent-specific UI
-// metadata (marketplace icons, display names). Only agents with
-// InstallAgentMetadata=true receive these during a copy install.
-var agentMetadataDirs = []string{"agents", "assets"}
-
-// isAgentMetadataPath reports whether rel (forward-slash separated) falls
-// under one of the agent metadata directories.
-func isAgentMetadataPath(rel string) bool {
-	normalized := filepath.ToSlash(rel)
-	for _, dir := range agentMetadataDirs {
-		if normalized == dir || strings.HasPrefix(normalized, dir+"/") {
-			return true
-		}
-	}
-	return false
-}
-
 func installSkillForAgents(ctx context.Context, skillName string, files []string, detectedAgents []*agents.Agent, params installParams) error {
 	canonicalDir := filepath.Join(params.baseDir, skillName)
 	if err := installSkillToDir(ctx, params.ref, skillName, canonicalDir, files); err != nil {
 		return err
 	}
 
 	// For project scope, always symlink. For global, symlink when multiple agents.
-	// Agents with PreferSkillCopy always get a full directory copy so
-	// marketplace metadata under agents/ and assets/ stays under their config dir.
+	// Symlinks point at the canonical tree (full manifest layout for every agent).
 	symlinkEligible := params.scope == ScopeProject || len(detectedAgents) > 1
 
 	for _, agent := range detectedAgents {
@@ -443,7 +433,7 @@ func installSkillForAgents(ctx context.Context, skillName string, files []string
 			continue
 		}
 
-		useSymlink := symlinkEligible && !agent.PreferSkillCopy
+		useSymlink := symlinkEligible
 		if useSymlink {
 			symlinkTarget := canonicalDir
 			// For project scope, use relative symlinks so they work for teammates.
@@ -455,17 +445,14 @@ func installSkillForAgents(ctx context.Context, skillName string, files []string
 			}
 			if err := createSymlink(symlinkTarget, destDir); err != nil {
 				log.Debugf(ctx, "Symlink failed for %s, copying instead: %v", agent.DisplayName, err)
-				if err := copyDir(canonicalDir, destDir); err != nil {
+				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 (symlinked)", skillName, agent.DisplayName)
 		} else {
-			// Copy from canonical dir. Skip agent metadata dirs for agents
-			// that don't consume them.
-			skipMetadata := !agent.InstallAgentMetadata
-			if err := copyDirFiltered(canonicalDir, destDir, skipMetadata); err != nil {
+			if err := copySkillMaterialized(canonicalDir, destDir); err != nil {
 				log.Warnf(ctx, "Failed to install for %s: %v", agent.DisplayName, err)
 				continue
 			}
@@ -509,6 +496,13 @@ func backupThirdPartySkill(ctx context.Context, destDir, canonicalDir, skillName
 		}
 	}
 
+	// Prior materialized install from this CLI; copySkillMaterialized will replace it.
+	if fi.IsDir() {
+		if _, err := os.Stat(filepath.Join(destDir, managedSkillMarker)); err == nil {
+			return nil
+		}
+	}
+
 	backupDir, err := os.MkdirTemp("", fmt.Sprintf("databricks-skill-backup-%s-*", skillName))
 	if err != nil {
 		return fmt.Errorf("failed to create backup directory: %w", err)
@@ -542,9 +536,15 @@ func installSkillToDir(ctx context.Context, ref, skillName, destDir string, file
 		// 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 {
+			if rootPath == "" {
+				return fmt.Errorf("invalid manifest file entry: empty path after %q", sharedFilePrefix)
+			}
 			file = rootPath
 		}
-		destPath := filepath.Join(destDir, file)
+		destPath, err := safeSkillDestPath(destDir, file)
+		if err != nil {
+			return err
+		}
 
 		if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil {
 			return fmt.Errorf("failed to create directory: %w", err)
@@ -559,20 +559,35 @@ func installSkillToDir(ctx context.Context, ref, skillName, destDir string, file
 	return nil
 }
 
-// copyDir copies all files from src to dest, recreating the directory structure.
-func copyDir(src, dest string) error {
-	return copyDirFiltered(src, dest, false)
+// safeSkillDestPath joins destDir with a manifest-relative path and rejects escapes.
+func safeSkillDestPath(destDir, file string) (string, error) {
+	if file == "" {
+		return "", errors.New("invalid manifest file entry: empty path")
+	}
+	destPath := filepath.Join(destDir, file)
+	base := filepath.Clean(destDir)
+	cleaned := filepath.Clean(destPath)
+	rel, err := filepath.Rel(base, cleaned)
+	if err != nil {
+		return "", fmt.Errorf("invalid skill file path %q: %w", file, err)
+	}
+	if rel == "." {
+		return "", fmt.Errorf("invalid skill file path %q: resolves to skill root", file)
+	}
+	if !filepath.IsLocal(rel) {
+		return "", fmt.Errorf("skill file path %q escapes destination directory", file)
+	}
+	return destPath, nil
 }
 
-// copyDirFiltered copies files from src to dest. When skipAgentMetadata is
-// true, subdirectories matching agentMetadataDirs (agents/, assets/) are
-// skipped so non-Codex agents don't receive marketplace UI files.
-func copyDirFiltered(src, dest string, skipAgentMetadata bool) error {
+// 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 {
 	if err := os.RemoveAll(dest); err != nil {
 		return fmt.Errorf("failed to remove existing path: %w", err)
 	}
 
-	return filepath.Walk(src, func(path string, info os.FileInfo, err error) error {
+	err := filepath.WalkDir(src, func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return err
 		}
@@ -582,16 +597,9 @@ func copyDirFiltered(src, dest string, skipAgentMetadata bool) error {
 			return err
 		}
 
-		if skipAgentMetadata && rel != "." && isAgentMetadataPath(rel) {
-			if info.IsDir() {
-				return filepath.SkipDir
-			}
-			return nil
-		}
-
 		target := filepath.Join(dest, rel)
 
-		if info.IsDir() {
+		if d.IsDir() {
 			return os.MkdirAll(target, 0o755)
 		}
 
@@ -604,6 +612,10 @@ func copyDirFiltered(src, dest string, skipAgentMetadata bool) error {
 		}
 		return os.WriteFile(target, data, 0o644)
 	})
+	if err != nil {
+		return err
+	}
+	return os.WriteFile(filepath.Join(dest, managedSkillMarker), nil, 0o644)
 }
 
 func createSymlink(source, dest string) error {