diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt b/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt index 71fa9e5604a..c1a6249bf76 100644 --- a/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt @@ -7,6 +7,4 @@ Installing Databricks skills for Claude Code... Using skills version test-ref 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 3200ae3727c..dc972dd17d3 100644 --- a/acceptance/experimental/aitools/skills/install-specific/output.txt +++ b/acceptance/experimental/aitools/skills/install-specific/output.txt @@ -6,8 +6,6 @@ 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 @@ -17,8 +15,6 @@ 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 diff --git a/acceptance/experimental/aitools/skills/install/output.txt b/acceptance/experimental/aitools/skills/install/output.txt index 77cd8551597..bcd43f22227 100644 --- a/acceptance/experimental/aitools/skills/install/output.txt +++ b/acceptance/experimental/aitools/skills/install/output.txt @@ -6,8 +6,6 @@ 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 @@ -17,8 +15,6 @@ 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) diff --git a/acceptance/experimental/aitools/skills/update-prune/output.txt b/acceptance/experimental/aitools/skills/update-prune/output.txt index 744e53b0d39..564957813a4 100644 --- a/acceptance/experimental/aitools/skills/update-prune/output.txt +++ b/acceptance/experimental/aitools/skills/update-prune/output.txt @@ -6,17 +6,11 @@ Flag --global has been deprecated, use --scope=global Installing Databricks skills for Cursor... 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 eca459e14e2..1102ae9d14e 100644 --- a/cmd/aitools/install.go +++ b/cmd/aitools/install.go @@ -18,6 +18,7 @@ import ( // install_test.go. var ( promptAgentSelection = defaultPromptAgentSelection + promptProceed = defaultPromptProceed installSkillsForAgentsFn = installer.InstallSkillsForAgents installPluginForAgentFn = installer.InstallPluginForAgent recordPluginInstallsFn = installer.RecordPluginInstalls @@ -140,7 +141,7 @@ Supported agents: Claude Code, Cursor, Codex CLI, OpenCode, GitHub Copilot, Anti // In the interactive picker path, show a plan summary and confirm. if !explicit && cmdio.IsPromptSupported(ctx) { printPlanSummary(ctx, plan, scope) - proceed, err := cmdio.AskYesOrNo(ctx, "Proceed?") + proceed, err := promptProceed(ctx) if err != nil { return err } @@ -247,6 +248,18 @@ func agentStateLabel(s agents.DisplayState) string { } } +func defaultPromptProceed(_ context.Context) (bool, error) { + proceed := true + err := huh.NewConfirm(). + Title("Proceed?"). + Value(&proceed). + Run() + if err != nil { + return false, err + } + return proceed, nil +} + func defaultPromptAgentSelection(_ context.Context, choices []agentChoice) ([]*agents.Agent, error) { options := make([]huh.Option[string], 0, len(choices)) byName := make(map[string]*agents.Agent, len(choices)) diff --git a/cmd/aitools/install_test.go b/cmd/aitools/install_test.go index bd3394eae37..21ac2ad5d56 100644 --- a/cmd/aitools/install_test.go +++ b/cmd/aitools/install_test.go @@ -321,6 +321,16 @@ func TestInstallInteractivePickerAndConfirm(t *testing.T) { return nil, nil } + // The confirm is a huh widget that reads the real terminal, so drive it via + // the override rather than piped stdin; assert it was consulted. + origProceed := promptProceed + t.Cleanup(func() { promptProceed = origProceed }) + proceedCalled := false + promptProceed = func(_ context.Context) (bool, error) { + proceedCalled = true + return true, nil + } + ctx, test := cmdio.SetupTest(t.Context(), cmdio.TestOptions{PromptSupported: true}) defer test.Done() go drainReader(test.Stdout) @@ -329,15 +339,9 @@ func TestInstallInteractivePickerAndConfirm(t *testing.T) { cmd := NewInstallCmd() cmd.SetContext(ctx) - errc := make(chan error, 1) - go func() { errc <- cmd.RunE(cmd, nil) }() - - _, err := test.Stdin.WriteString("y\n") - require.NoError(t, err) - require.NoError(t, test.Stdin.Flush()) - - require.NoError(t, <-errc) + require.NoError(t, cmd.RunE(cmd, nil)) assert.True(t, pickerCalled) + assert.True(t, proceedCalled) require.Len(t, *plugins, 1) assert.Equal(t, agents.NameClaudeCode, (*plugins)[0].agent) } diff --git a/libs/aitools/installer/installer.go b/libs/aitools/installer/installer.go index eb627ad3ed0..93521292735 100644 --- a/libs/aitools/installer/installer.go +++ b/libs/aitools/installer/installer.go @@ -316,14 +316,15 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent return fmt.Errorf("failed to load install state: %w", err) } - // Detect legacy installs (skills on disk but no state file). Global only. - // Block targeted installs on legacy setups to avoid writing incomplete state - // that would hide the legacy warning on future runs. - if state == nil && scope == ScopeGlobal { - isLegacy := checkLegacyInstall(ctx, baseDir) - if isLegacy && len(opts.SpecificSkills) > 0 { - return errors.New("legacy install detected without state tracking; run 'databricks aitools install' (without a skill name) first to rebuild state") - } + // A "legacy install" is one from before the CLI tracked install state in a + // JSON file: skills exist on disk but there's no state file (state == nil). + // A targeted install (--skills foo) here would write a state file listing only + // foo, leaving the other on-disk skills untracked. Block it so the user runs a + // full install first, which rebuilds complete state. Global scope only, since + // legacy installs only ever wrote to the global dir. + // hasLegacyInstall lives in update.go (shared with the update/uninstall paths). + if state == nil && scope == ScopeGlobal && len(opts.SpecificSkills) > 0 && hasLegacyInstall(ctx, baseDir) { + return errors.New("legacy install detected without state tracking; run 'databricks aitools install' (without a skill name) first to rebuild state") } // Filter skills based on options, experimental flag, and CLI version. @@ -342,6 +343,11 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent // Install each skill in sorted order for determinism. skillNames := slices.Sorted(maps.Keys(targetSkills)) + // Scoped to the loop, not the manifest fetch above, so earlier warnings print + // as plain lines instead of racing the live spinner's redraw. + sp := cmdio.NewSpinner(ctx) + defer sp.Close() + // Accumulate file provenance for skills we (re)fetch this run. Skipped // (already-installed) skills keep their existing records via the merge below. fileRecords := map[string]FileRecord{} @@ -360,6 +366,7 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent } } + sp.Update("Installing " + name + "...") records, err := installSkillForAgents(ctx, name, meta, targetAgents, params) if err != nil { return err @@ -407,6 +414,10 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent return err } + // Stop the spinner before the summary so its transient line doesn't linger + // above the persistent "Installed N skills." output. + sp.Close() + noun := "skills" if len(targetSkills) == 1 { noun = "skill" @@ -522,25 +533,6 @@ func printNoAgentsDetected(ctx context.Context) { cmdio.LogString(ctx, "Please install at least one coding agent first.") } -// checkLegacyInstall prints a message if skills exist on disk but no state file was found. -// Returns true if a legacy install was detected. -func checkLegacyInstall(ctx context.Context, globalDir string) bool { - if hasSkillsOnDisk(globalDir) { - cmdio.LogString(ctx, "Found skills installed before state tracking was added. Run 'databricks aitools install' to refresh.") - return true - } - homeDir, err := env.UserHomeDir(ctx) - if err != nil { - return false - } - legacyDir := filepath.Join(homeDir, ".databricks", "agent-skills") - if hasSkillsOnDisk(legacyDir) { - cmdio.LogString(ctx, "Found skills installed before state tracking was added. Run 'databricks aitools install' to refresh.") - return true - } - return false -} - // hasSkillsOnDisk checks if a directory contains subdirectories starting with "databricks". func hasSkillsOnDisk(dir string) bool { entries, err := os.ReadDir(dir) @@ -580,7 +572,6 @@ 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 @@ -588,7 +579,6 @@ 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) @@ -634,13 +624,6 @@ 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 { diff --git a/libs/aitools/installer/installer_test.go b/libs/aitools/installer/installer_test.go index 13c870cbebb..2502c113db5 100644 --- a/libs/aitools/installer/installer_test.go +++ b/libs/aitools/installer/installer_test.go @@ -439,9 +439,10 @@ 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) + // Per-skill progress now goes through a spinner, which is silent in this + // non-interactive test, so only these plain lines remain. + assert.Contains(t, stderr.String(), "Using skills version") 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.") } @@ -713,42 +714,26 @@ func TestIdempotentInstallUpdatesNewVersions(t *testing.T) { assert.Equal(t, "0.2.0", state.Skills["databricks-sql"]) } -func TestLegacyDetectMessagePrinted(t *testing.T) { +func TestLegacyTargetedInstallBlockedFromLegacyDir(t *testing.T) { tmp := setupTestHome(t) - ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) - setupFetchMock(t) - t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) - - // Create skills on disk at canonical location but no state file. - globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") - require.NoError(t, os.MkdirAll(filepath.Join(globalDir, "databricks-sql"), 0o755)) - - src := &mockManifestSource{manifest: testManifest()} - agent := testAgent(tmp) - - err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) - require.NoError(t, err) - - assert.Contains(t, stderr.String(), "Found skills installed before state tracking was added.") -} - -func TestLegacyDetectLegacyDir(t *testing.T) { - tmp := setupTestHome(t) - ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) - // Create skills in the legacy location. + // Skills in the legacy location (~/.databricks/agent-skills), no state file. legacyDir := filepath.Join(tmp, ".databricks", "agent-skills") require.NoError(t, os.MkdirAll(filepath.Join(legacyDir, "databricks-sql"), 0o755)) src := &mockManifestSource{manifest: testManifest()} agent := testAgent(tmp) - err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) - require.NoError(t, err) - - assert.Contains(t, stderr.String(), "Found skills installed before state tracking was added.") + // A targeted install must be blocked when a legacy install is detected via + // the legacy dir, mirroring the canonical-dir case. + err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{ + SpecificSkills: []string{"databricks-sql"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "legacy install detected") } func TestIdempotentInstallReinstallsForNewAgent(t *testing.T) { @@ -831,11 +816,11 @@ func TestLegacyFullInstallAllowed(t *testing.T) { src := &mockManifestSource{manifest: testManifest()} agent := testAgent(tmp) - // Full install (no SpecificSkills) should succeed and rebuild state. + // Full install (no SpecificSkills) should succeed and rebuild state silently. err := InstallSkillsForAgents(ctx, src, []*agents.Agent{agent}, InstallOptions{}) require.NoError(t, err) - assert.Contains(t, stderr.String(), "Found skills installed before state tracking was added.") + assert.Contains(t, stderr.String(), "Installed 2 skills.") state, err := LoadState(globalDir) require.NoError(t, err) diff --git a/libs/aitools/installer/update.go b/libs/aitools/installer/update.go index 0b1e30efcf6..36a7ef9c279 100644 --- a/libs/aitools/installer/update.go +++ b/libs/aitools/installer/update.go @@ -187,15 +187,20 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent ref: latestTag, } + sp := cmdio.NewSpinner(ctx) + defer sp.Close() + fileRecords := map[string]FileRecord{} for _, change := range allChanges { meta := manifest.Skills[change.Name] + sp.Update("Installing " + change.Name + "...") records, err := installSkillForAgents(ctx, change.Name, meta, targetAgents, params) if err != nil { return nil, err } maps.Copy(fileRecords, records) } + sp.Close() // Update state. state.Release = latestTag