fixup: address Simon's review

- Restore the pre-refactor validation that combining --project and --global
  on `list` is an error, not a silent equivalent of --scope=both. Moved the
  check into parseScopeFlag itself so it applies uniformly across commands.
- For commands that pass allowBoth=false (install, uninstall), the
  invalid-value error no longer mentions "both" as a valid option.
- Added TestUpdateScopeFlag and TestUninstallScopeFlag covering the new
  Cobra wiring (--scope=global/both, invalid value, legacy-flag conflict,
  and both-deprecated-flags-together cases). Introduced updateSkillsFn and
  uninstallSkillsFn package-level test-injection vars mirroring the
  existing installSkillsForAgentsFn pattern in install.go.

Co-authored-by: Isaac

Author: jamesbroadhead

cmd/aitools/list_test.go

@@ -57,7 +57,7 @@ func TestListScopeFlag(t *testing.T) {
 		{name: "scope global", args: []string{"--scope", "global"}, wantScope: installer.ScopeGlobal},
 		{name: "scope both shows both", args: []string{"--scope", "both"}, wantScope: ""},
 		{name: "scope invalid", args: []string{"--scope", "all"}, wantErr: `invalid --scope "all"`},
-		{name: "legacy both flags shows both", args: []string{"--project", "--global"}, wantScope: ""},
+		{name: "legacy both flags together rejected", args: []string{"--project", "--global"}, wantErr: "cannot use --global and --project together"},
 	}
 
 	for _, tt := range tests {

cmd/aitools/scope.go

@@ -97,11 +97,18 @@ func markScopeBoolsDeprecated(cmd *cobra.Command) {
 // parseScopeFlag translates --scope into the equivalent --project/--global bool pair.
 // Returns (projectFlag, globalFlag, nil) unchanged when --scope is empty so the
 // deprecated booleans can keep flowing through the existing resolveScope* helpers.
-// Errors if --scope is combined with --project or --global. When allowBoth is
+// Errors if --scope is combined with --project or --global, or if both deprecated
+// flags are set together (matching the pre-refactor validation). When allowBoth is
 // false, --scope=both is rejected up front so install and uninstall don't have
 // to special-case it.
 func parseScopeFlag(scopeFlag string, projectFlag, globalFlag, allowBoth bool) (proj, glob bool, err error) {
 	if scopeFlag == "" {
+		// Preserve the pre-refactor behavior: combining the two deprecated flags
+		// is always wrong, regardless of allowBoth. Users who want both scopes
+		// should use --scope=both (where supported).
+		if projectFlag && globalFlag {
+			return false, false, errors.New("cannot use --global and --project together")
+		}
 		return projectFlag, globalFlag, nil
 	}
 	if projectFlag || globalFlag {
@@ -118,7 +125,10 @@ func parseScopeFlag(scopeFlag string, projectFlag, globalFlag, allowBoth bool) (
 		}
 		return true, true, nil
 	default:
-		return false, false, fmt.Errorf("invalid --scope %q: must be one of project, global, both", scopeFlag)
+		if allowBoth {
+			return false, false, fmt.Errorf("invalid --scope %q: must be one of project, global, both", scopeFlag)
+		}
+		return false, false, fmt.Errorf("invalid --scope %q: must be one of project, global", scopeFlag)
 	}
 }
 

cmd/aitools/scope_test.go

@@ -83,12 +83,13 @@ func TestParseScopeFlag(t *testing.T) {
 		{name: "unset", scope: ""},
 		{name: "legacy project only", project: true, wantProj: true},
 		{name: "legacy global only", global: true, wantGlob: true},
-		{name: "legacy both passthrough", project: true, global: true, wantProj: true, wantGlob: true},
+		{name: "legacy both flags together rejected", project: true, global: true, wantErr: "cannot use --global and --project together"},
 		{name: "scope project", scope: "project", wantProj: true},
 		{name: "scope global", scope: "global", wantGlob: true},
 		{name: "scope both allowed", scope: "both", allowBoth: true, wantProj: true, wantGlob: true},
 		{name: "scope both disallowed", scope: "both", wantErr: "--scope=both is not supported"},
-		{name: "scope invalid value", scope: "all", wantErr: `invalid --scope "all"`},
+		{name: "scope invalid value with allowBoth", scope: "all", allowBoth: true, wantErr: `invalid --scope "all": must be one of project, global, both`},
+		{name: "scope invalid value without allowBoth omits both from error", scope: "all", wantErr: `invalid --scope "all": must be one of project, global`},
 		{name: "scope conflicts with project", scope: "project", project: true, wantErr: "cannot use --scope with --project or --global"},
 		{name: "scope conflicts with global", scope: "global", global: true, wantErr: "cannot use --scope with --project or --global"},
 	}
@@ -106,6 +107,15 @@ func TestParseScopeFlag(t *testing.T) {
 			assert.Equal(t, tt.wantGlob, glob)
 		})
 	}
+
+	// Stronger check that the without-allowBoth invalid-value branch omits
+	// "both" from the error message (the table assertion uses Contains which
+	// can't distinguish a substring shared with the allowBoth variant).
+	t.Run("invalid scope error message without allowBoth does not mention both", func(t *testing.T) {
+		_, _, err := parseScopeFlag("all", false, false, false)
+		require.Error(t, err)
+		assert.NotContains(t, err.Error(), "both")
+	})
 }
 
 // --- detectInstalledScopes tests (table-driven) ---

cmd/aitools/uninstall.go

@@ -1,10 +1,17 @@
 package aitools
 
 import (
+	"context"
+
 	"github.com/databricks/cli/libs/aitools/installer"
 	"github.com/spf13/cobra"
 )
 
+// Package-level for testability. Tests override via uninstall_test.go.
+var uninstallSkillsFn = func(ctx context.Context, opts installer.UninstallOptions) error {
+	return installer.UninstallSkillsOpts(ctx, opts)
+}
+
 func NewUninstallCmd() *cobra.Command {
 	var skillsFlag, scopeFlag string
 	var projectFlag, globalFlag bool
@@ -42,7 +49,7 @@ By default, removes all skills. Use --skills to remove specific skills only.`,
 				Scope: scope,
 			}
 			opts.Skills = splitAndTrim(skillsFlag)
-			return installer.UninstallSkillsOpts(ctx, opts)
+			return uninstallSkillsFn(ctx, opts)
 		},
 	}
 

cmd/aitools/uninstall_test.go

@@ -0,0 +1,66 @@
+package aitools
+
+import (
+	"context"
+	"testing"
+
+	"github.com/databricks/cli/libs/aitools/installer"
+	"github.com/databricks/cli/libs/cmdio"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func setupUninstallMock(t *testing.T) *[]installer.UninstallOptions {
+	t.Helper()
+	orig := uninstallSkillsFn
+	t.Cleanup(func() { uninstallSkillsFn = orig })
+
+	var calls []installer.UninstallOptions
+	uninstallSkillsFn = func(_ context.Context, opts installer.UninstallOptions) error {
+		calls = append(calls, opts)
+		return nil
+	}
+	return &calls
+}
+
+func TestUninstallScopeFlag(t *testing.T) {
+	tests := []struct {
+		name      string
+		args      []string
+		wantScope string
+		wantErr   string
+	}{
+		// scope=project requires installed project state and is covered via TestParseScopeFlag
+		// and TestResolveScopeForUninstallProjectFlagWithState. Here we cover the no-state paths
+		// and the failure modes specific to the Cobra wiring.
+		{name: "scope global", args: []string{"--scope", "global"}, wantScope: installer.ScopeGlobal},
+		{name: "scope both rejected", args: []string{"--scope", "both"}, wantErr: "--scope=both is not supported"},
+		{name: "scope invalid value", args: []string{"--scope", "all"}, wantErr: `invalid --scope "all"`},
+		{name: "scope conflicts with legacy project", args: []string{"--scope", "global", "--project"}, wantErr: "cannot use --scope with --project or --global"},
+		{name: "legacy both flags together rejected", args: []string{"--project", "--global"}, wantErr: "cannot use --global and --project together"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			setupTestAgents(t)
+			calls := setupUninstallMock(t)
+
+			ctx := cmdio.MockDiscard(t.Context())
+			cmd := NewUninstallCmd()
+			cmd.SetContext(ctx)
+			cmd.SetArgs(tt.args)
+			cmd.SilenceErrors = true
+			cmd.SilenceUsage = true
+
+			err := cmd.Execute()
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			require.Len(t, *calls, 1)
+			assert.Equal(t, tt.wantScope, (*calls)[0].Scope)
+		})
+	}
+}

cmd/aitools/update.go

@@ -1,6 +1,7 @@
 package aitools
 
 import (
+	"context"
 	"fmt"
 
 	"github.com/databricks/cli/libs/aitools/agents"
@@ -9,6 +10,11 @@ import (
 	"github.com/spf13/cobra"
 )
 
+// Package-level for testability. Tests override via update_test.go.
+var updateSkillsFn = func(ctx context.Context, src installer.ManifestSource, installed []*agents.Agent, opts installer.UpdateOptions) (*installer.UpdateResult, error) {
+	return installer.UpdateSkills(ctx, src, installed, opts)
+}
+
 func NewUpdateCmd() *cobra.Command {
 	var check, force, noNew bool
 	var skillsFlag, scopeFlag string
@@ -62,7 +68,7 @@ preview what would change without downloading.`,
 				}
 				opts.Skills = skills
 
-				result, err := installer.UpdateSkills(ctx, src, installed, opts)
+				result, err := updateSkillsFn(ctx, src, installed, opts)
 				if err != nil {
 					return err
 				}

cmd/aitools/update_test.go

@@ -0,0 +1,69 @@
+package aitools
+
+import (
+	"context"
+	"testing"
+
+	"github.com/databricks/cli/libs/aitools/agents"
+	"github.com/databricks/cli/libs/aitools/installer"
+	"github.com/databricks/cli/libs/cmdio"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func setupUpdateMock(t *testing.T) *[]installer.UpdateOptions {
+	t.Helper()
+	orig := updateSkillsFn
+	t.Cleanup(func() { updateSkillsFn = orig })
+
+	var calls []installer.UpdateOptions
+	updateSkillsFn = func(_ context.Context, _ installer.ManifestSource, _ []*agents.Agent, opts installer.UpdateOptions) (*installer.UpdateResult, error) {
+		calls = append(calls, opts)
+		return &installer.UpdateResult{}, nil
+	}
+	return &calls
+}
+
+func TestUpdateScopeFlag(t *testing.T) {
+	tests := []struct {
+		name       string
+		args       []string
+		wantScopes []string
+		wantErr    string
+	}{
+		// scope=project requires installed project state and is covered via TestParseScopeFlag
+		// (cmd/aitools/scope_test.go) and TestResolveScopeForUpdateProjectFlagWithState. Here
+		// we cover the no-state paths and the failure modes specific to the Cobra wiring.
+		{name: "scope global", args: []string{"--scope", "global"}, wantScopes: []string{installer.ScopeGlobal}},
+		{name: "scope both with no installs falls through to global", args: []string{"--scope", "both"}, wantScopes: []string{installer.ScopeGlobal}},
+		{name: "scope invalid value", args: []string{"--scope", "all"}, wantErr: `invalid --scope "all"`},
+		{name: "scope conflicts with legacy project", args: []string{"--scope", "global", "--project"}, wantErr: "cannot use --scope with --project or --global"},
+		{name: "legacy both flags together rejected", args: []string{"--project", "--global"}, wantErr: "cannot use --global and --project together"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			setupTestAgents(t)
+			calls := setupUpdateMock(t)
+
+			ctx := cmdio.MockDiscard(t.Context())
+			cmd := NewUpdateCmd()
+			cmd.SetContext(ctx)
+			cmd.SetArgs(tt.args)
+			cmd.SilenceErrors = true
+			cmd.SilenceUsage = true
+
+			err := cmd.Execute()
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			require.Len(t, *calls, len(tt.wantScopes))
+			for i, scope := range tt.wantScopes {
+				assert.Equal(t, scope, (*calls)[i].Scope)
+			}
+		})
+	}
+}