Address review findings: relative symlinks, project scope list, zero-agent guard, param cleanup

Co-authored-by: Isaac

Author: simonfaltum

experimental/aitools/cmd/list.go

@@ -1,6 +1,7 @@
 package aitools
 
 import (
+	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -17,6 +18,8 @@ import (
 var listSkillsFn = defaultListSkills
 
 func newListCmd() *cobra.Command {
+	var projectFlag, globalFlag bool
+
 	// --skills is accepted for forward-compat (future component types)
 	// but currently skills is the only component, so the output is the same.
 	var showSkills bool
@@ -25,16 +28,27 @@ func newListCmd() *cobra.Command {
 		Use:   "list",
 		Short: "List installed AI tools components",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			_ = showSkills
-			return listSkillsFn(cmd)
+			if projectFlag && globalFlag {
+				return errors.New("cannot use --global and --project together")
+			}
+			// For list: no flag = show both scopes (empty string).
+			var scope string
+			if projectFlag {
+				scope = installer.ScopeProject
+			} else if globalFlag {
+				scope = installer.ScopeGlobal
+			}
+			return listSkillsFn(cmd, scope)
 		},
 	}
 
 	cmd.Flags().BoolVar(&showSkills, "skills", false, "Show detailed skills information")
+	cmd.Flags().BoolVar(&projectFlag, "project", false, "Show only project-scoped skills")
+	cmd.Flags().BoolVar(&globalFlag, "global", false, "Show only globally-scoped skills")
 	return cmd
 }
 
-func defaultListSkills(cmd *cobra.Command) error {
+func defaultListSkills(cmd *cobra.Command, scope string) error {
 	ctx := cmd.Context()
 
 	src := &installer.GitHubManifestSource{}
@@ -48,14 +62,28 @@ func defaultListSkills(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to fetch manifest: %w", err)
 	}
 
-	globalDir, err := installer.GlobalSkillsDir(ctx)
-	if err != nil {
-		return err
+	// Load global state.
+	var globalState *installer.InstallState
+	if scope != installer.ScopeProject {
+		globalDir, gErr := installer.GlobalSkillsDir(ctx)
+		if gErr == nil {
+			globalState, err = installer.LoadState(globalDir)
+			if err != nil {
+				log.Debugf(ctx, "Could not load global install state: %v", err)
+			}
+		}
 	}
 
-	state, err := installer.LoadState(globalDir)
-	if err != nil {
-		log.Debugf(ctx, "Could not load install state: %v", err)
+	// Load project state.
+	var projectState *installer.InstallState
+	if scope != installer.ScopeGlobal {
+		projectDir, pErr := installer.ProjectSkillsDir(ctx)
+		if pErr == nil {
+			projectState, err = installer.LoadState(projectDir)
+			if err != nil {
+				log.Debugf(ctx, "Could not load project install state: %v", err)
+			}
+		}
 	}
 
 	// Build sorted list of skill names.
@@ -73,7 +101,10 @@ func defaultListSkills(cmd *cobra.Command) error {
 	tw := tabwriter.NewWriter(&buf, 0, 4, 2, ' ', 0)
 	fmt.Fprintln(tw, "  NAME\tVERSION\tINSTALLED")
 
-	installedCount := 0
+	bothScopes := globalState != nil && projectState != nil
+
+	globalCount := 0
+	projectCount := 0
 	for _, name := range names {
 		meta := manifest.Skills[name]
 
@@ -82,15 +113,15 @@ func defaultListSkills(cmd *cobra.Command) error {
 			tag = " [experimental]"
 		}
 
-		installedStr := "not installed"
-		if state != nil {
-			if v, ok := state.Skills[name]; ok {
-				installedCount++
-				if v == meta.Version {
-					installedStr = "v" + v + " (up to date)"
-				} else {
-					installedStr = "v" + v + " (update available)"
-				}
+		installedStr := installedStatus(name, meta.Version, globalState, projectState, bothScopes)
+		if globalState != nil {
+			if _, ok := globalState.Skills[name]; ok {
+				globalCount++
+			}
+		}
+		if projectState != nil {
+			if _, ok := projectState.Skills[name]; ok {
+				projectCount++
 			}
 		}
 
@@ -99,6 +130,59 @@ func defaultListSkills(cmd *cobra.Command) error {
 	tw.Flush()
 	cmdio.LogString(ctx, buf.String())
 
-	cmdio.LogString(ctx, fmt.Sprintf("%d/%d skills installed (global)", installedCount, len(names)))
+	// Summary line.
+	switch {
+	case bothScopes:
+		cmdio.LogString(ctx, fmt.Sprintf("%d/%d skills installed (global), %d/%d (project)", globalCount, len(names), projectCount, len(names)))
+	case projectState != nil:
+		cmdio.LogString(ctx, fmt.Sprintf("%d/%d skills installed (project)", projectCount, len(names)))
+	default:
+		installedCount := globalCount
+		cmdio.LogString(ctx, fmt.Sprintf("%d/%d skills installed (global)", installedCount, len(names)))
+	}
 	return nil
 }
+
+// installedStatus returns the display string for a skill's installation status.
+func installedStatus(name, latestVersion string, globalState, projectState *installer.InstallState, bothScopes bool) string {
+	globalVer := ""
+	projectVer := ""
+
+	if globalState != nil {
+		globalVer = globalState.Skills[name]
+	}
+	if projectState != nil {
+		projectVer = projectState.Skills[name]
+	}
+
+	if globalVer == "" && projectVer == "" {
+		return "not installed"
+	}
+
+	// If both scopes have the skill, show the project version (takes precedence).
+	if bothScopes && globalVer != "" && projectVer != "" {
+		return versionLabel(projectVer, latestVersion) + " (project, global)"
+	}
+
+	if projectVer != "" {
+		label := versionLabel(projectVer, latestVersion)
+		if bothScopes {
+			return label + " (project)"
+		}
+		return label
+	}
+
+	label := versionLabel(globalVer, latestVersion)
+	if bothScopes {
+		return label + " (global)"
+	}
+	return label
+}
+
+// versionLabel formats version with update status.
+func versionLabel(installed, latest string) string {
+	if installed == latest {
+		return "v" + installed + " (up to date)"
+	}
+	return "v" + installed + " (update available)"
+}

experimental/aitools/cmd/list_test.go

@@ -19,7 +19,7 @@ func TestListCommandCallsListFn(t *testing.T) {
 	t.Cleanup(func() { listSkillsFn = orig })
 
 	called := false
-	listSkillsFn = func(cmd *cobra.Command) error {
+	listSkillsFn = func(cmd *cobra.Command, scope string) error {
 		called = true
 		return nil
 	}
@@ -40,12 +40,20 @@ func TestListCommandHasSkillsFlag(t *testing.T) {
 	assert.Equal(t, "false", f.DefValue)
 }
 
+func TestListCommandHasScopeFlags(t *testing.T) {
+	cmd := newListCmd()
+	f := cmd.Flags().Lookup("project")
+	require.NotNil(t, f, "--project flag should exist")
+	f = cmd.Flags().Lookup("global")
+	require.NotNil(t, f, "--global flag should exist")
+}
+
 func TestSkillsListDelegatesToListFn(t *testing.T) {
 	orig := listSkillsFn
 	t.Cleanup(func() { listSkillsFn = orig })
 
 	called := false
-	listSkillsFn = func(cmd *cobra.Command) error {
+	listSkillsFn = func(cmd *cobra.Command, scope string) error {
 		called = true
 		return nil
 	}

experimental/aitools/cmd/skills.go

@@ -66,7 +66,8 @@ func newSkillsListCmd() *cobra.Command {
 		Use:   "list",
 		Short: "List available skills",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return listSkillsFn(cmd)
+			// Default to showing all scopes (empty scope = both).
+			return listSkillsFn(cmd, "")
 		},
 	}
 }

experimental/aitools/cmd/version.go

@@ -106,4 +106,10 @@ func printVersionLine(ctx context.Context, label string, state *installer.Instal
 		latestVersion := strings.TrimPrefix(latest, "v")
 		cmdio.LogString(ctx, fmt.Sprintf("  %s: v%s (%d %s, update available: v%s)", label, version, len(state.Skills), skillNoun, latestVersion))
 	}
+
+	cmdio.LogString(ctx, "  Last updated: "+state.LastUpdated.Format("2006-01-02"))
+
+	if authoritative && latest != state.Release {
+		cmdio.LogString(ctx, "  Run 'databricks experimental aitools update' to update.")
+	}
 }

experimental/aitools/cmd/version_test.go

@@ -64,6 +64,7 @@ func TestVersionShowsBothScopes(t *testing.T) {
 	assert.Contains(t, output, "v0.2.0")
 	assert.Contains(t, output, "2 skills")
 	assert.Contains(t, output, "3 skills")
+	assert.Contains(t, output, "Last updated: 2026-03-22")
 }
 
 func TestVersionShowsSingleScopeWithoutQualifier(t *testing.T) {

experimental/aitools/lib/installer/installer.go

@@ -125,7 +125,11 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent
 		if err != nil {
 			return fmt.Errorf("failed to determine working directory: %w", err)
 		}
+		incompatible := incompatibleAgentNames(targetAgents)
 		targetAgents = filterProjectAgents(ctx, targetAgents)
+		if len(targetAgents) == 0 {
+			return fmt.Errorf("no agents support project-scoped skills. The following detected agents are global-only: %s", strings.Join(incompatible, ", "))
+		}
 	}
 
 	// Load existing state for idempotency checks.
@@ -150,6 +154,13 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent
 		return err
 	}
 
+	params := installParams{
+		baseDir: baseDir,
+		scope:   scope,
+		cwd:     cwd,
+		ref:     latestTag,
+	}
+
 	// Install each skill in sorted order for determinism.
 	skillNames := make([]string, 0, len(targetSkills))
 	for name := range targetSkills {
@@ -169,7 +180,7 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent
 			}
 		}
 
-		if err := installSkillForAgents(ctx, latestTag, name, meta.Files, targetAgents, baseDir, scope, cwd); err != nil {
+		if err := installSkillForAgents(ctx, name, meta.Files, targetAgents, params); err != nil {
 			return err
 		}
 	}
@@ -226,6 +237,17 @@ func filterProjectAgents(ctx context.Context, targetAgents []*agents.Agent) []*a
 	return compatible
 }
 
+// incompatibleAgentNames returns the display names of agents that do not support project scope.
+func incompatibleAgentNames(targetAgents []*agents.Agent) []string {
+	var names []string
+	for _, a := range targetAgents {
+		if !a.SupportsProjectScope {
+			names = append(names, a.DisplayName)
+		}
+	}
+	return names
+}
+
 // resolveSkills filters the manifest skills based on the install options,
 // experimental flag, and CLI version constraints.
 func resolveSkills(ctx context.Context, skills map[string]SkillMeta, opts InstallOptions) (map[string]SkillMeta, error) {
@@ -362,17 +384,25 @@ func allAgentsHaveSkill(ctx context.Context, skillName string, targetAgents []*a
 	return true
 }
 
-func installSkillForAgents(ctx context.Context, ref, skillName string, files []string, detectedAgents []*agents.Agent, baseDir, scope, cwd string) error {
-	canonicalDir := filepath.Join(baseDir, skillName)
-	if err := installSkillToDir(ctx, ref, skillName, canonicalDir, files); err != nil {
+// installParams bundles the parameters for installSkillForAgents to keep the signature manageable.
+type installParams struct {
+	baseDir string
+	scope   string
+	cwd     string
+	ref     string
+}
+
+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.
-	useSymlinks := scope == ScopeProject || len(detectedAgents) > 1
+	useSymlinks := params.scope == ScopeProject || len(detectedAgents) > 1
 
 	for _, agent := range detectedAgents {
-		agentSkillDir, err := agentSkillsDirForScope(ctx, agent, scope, cwd)
+		agentSkillDir, err := agentSkillsDirForScope(ctx, agent, params.scope, params.cwd)
 		if err != nil {
 			log.Warnf(ctx, "Skipped %s: %v", agent.DisplayName, err)
 			continue
@@ -386,7 +416,15 @@ func installSkillForAgents(ctx context.Context, ref, skillName string, files []s
 		}
 
 		if useSymlinks {
-			if err := createSymlink(canonicalDir, destDir); err != nil {
+			symlinkTarget := canonicalDir
+			// For project scope, use relative symlinks so they work for teammates.
+			if params.scope == ScopeProject {
+				rel, relErr := filepath.Rel(filepath.Dir(destDir), canonicalDir)
+				if relErr == nil {
+					symlinkTarget = rel
+				}
+			}
+			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 {
 					log.Warnf(ctx, "Failed to install for %s: %v", agent.DisplayName, err)
@@ -429,8 +467,14 @@ func backupThirdPartySkill(ctx context.Context, destDir, canonicalDir, skillName
 	// If it's a symlink to our canonical dir, no backup needed.
 	if fi.Mode()&os.ModeSymlink != 0 {
 		target, err := os.Readlink(destDir)
-		if err == nil && target == canonicalDir {
-			return nil
+		if err == nil {
+			absTarget := target
+			if !filepath.IsAbs(target) {
+				absTarget = filepath.Clean(filepath.Join(filepath.Dir(destDir), target))
+			}
+			if absTarget == canonicalDir {
+				return nil
+			}
 		}
 	}