From a8c168912b15656748a155a778f99def23b4d1ef Mon Sep 17 00:00:00 2001 From: James Broadhead Date: Mon, 18 May 2026 22:06:49 +0000 Subject: [PATCH 1/5] aitools: parse experimental_skills manifest section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Teaches the aitools installer to parse the new manifest shape from databricks/databricks-agent-skills (paired with d-a-s #73): - Single `skills` map; each entry's `repo_dir` ("skills" or "experimental") is the source of truth for stable-vs-experimental. - `SkillMeta.IsExperimental()` derives state from `RepoDir`. - Experimental skills get a `-experimental` suffix on their install-side key in `normalizeManifest`; new `SourceName` field preserves the unsuffixed name for fetch URLs. - The existing `--experimental` flag (already wired in `cmd/skills.go`) now has skills to install; `resolveSkills` filters them otherwise. - New acceptance tests at `acceptance/experimental/aitools/skills/install*/` cover stable-only, --experimental, specific-skill, and empty-manifest cases. `DATABRICKS_SKILLS_BASE_URL` env-var lets the tests mock the manifest server. Rebased onto current main; layout drift reconciled from `experimental/aitools/lib/installer/` → `libs/aitools/installer/` and `experimental/aitools/cmd/list.go` → `cmd/aitools/list.go` (per #4917's final shape). Acceptance test golden files regenerated to capture the legacy-alias deprecation warning that #4917 added on `databricks experimental aitools install`. Co-authored-by: Isaac --- Taskfile.yml | 3 +- .../install-experimental-empty/out.test.toml | 3 + .../install-experimental-empty/output.txt | 8 ++ .../skills/install-experimental-empty/script | 4 + .../install-experimental-empty/test.toml | 32 +++++ .../skills/install-specific/out.test.toml | 3 + .../skills/install-specific/output.txt | 23 ++++ .../aitools/skills/install-specific/script | 10 ++ .../aitools/skills/install-specific/test.toml | 50 +++++++ .../aitools/skills/install/out.test.toml | 3 + .../aitools/skills/install/output.txt | 21 +++ .../aitools/skills/install/script | 11 ++ .../aitools/skills/install/test.toml | 59 +++++++++ cmd/aitools/list.go | 2 +- libs/aitools/installer/installer.go | 125 +++++++++++++++--- libs/aitools/installer/installer_test.go | 74 +++++++++-- libs/aitools/installer/source.go | 31 ++++- libs/aitools/installer/source_test.go | 73 ++++++++++ libs/aitools/installer/uninstall.go | 23 +++- libs/aitools/installer/uninstall_test.go | 75 +++++++++++ libs/aitools/installer/update.go | 4 +- libs/aitools/installer/update_test.go | 14 +- 22 files changed, 601 insertions(+), 50 deletions(-) create mode 100644 acceptance/experimental/aitools/skills/install-experimental-empty/out.test.toml create mode 100644 acceptance/experimental/aitools/skills/install-experimental-empty/output.txt create mode 100644 acceptance/experimental/aitools/skills/install-experimental-empty/script create mode 100644 acceptance/experimental/aitools/skills/install-experimental-empty/test.toml create mode 100644 acceptance/experimental/aitools/skills/install-specific/out.test.toml create mode 100644 acceptance/experimental/aitools/skills/install-specific/output.txt create mode 100644 acceptance/experimental/aitools/skills/install-specific/script create mode 100644 acceptance/experimental/aitools/skills/install-specific/test.toml create mode 100644 acceptance/experimental/aitools/skills/install/out.test.toml create mode 100644 acceptance/experimental/aitools/skills/install/output.txt create mode 100644 acceptance/experimental/aitools/skills/install/script create mode 100644 acceptance/experimental/aitools/skills/install/test.toml create mode 100644 libs/aitools/installer/source_test.go diff --git a/Taskfile.yml b/Taskfile.yml index 0d582a1954e..7a8fb12adef 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -615,6 +615,7 @@ tasks: - libs/aitools/** - experimental/aitools/** - acceptance/apps/** + - acceptance/experimental/aitools/** - "{{.EMBED_SOURCES}}" cmds: - | @@ -628,7 +629,7 @@ tasks: --format ${GOTESTSUM_FORMAT:-pkgname-and-test-fails} \ --no-summary=skipped \ --packages ./acceptance/... \ - -- -timeout=${LOCAL_TIMEOUT:-30m} -run "TestAccept/apps" + -- -timeout=${LOCAL_TIMEOUT:-30m} -run "TestAccept/(apps|experimental/aitools)" test-exp-ssh: desc: Run experimental SSH unit and acceptance tests diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/out.test.toml b/acceptance/experimental/aitools/skills/install-experimental-empty/out.test.toml new file mode 100644 index 00000000000..d6187dcb046 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt b/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt new file mode 100644 index 00000000000..776e8c85ffb --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt @@ -0,0 +1,8 @@ + +=== --experimental against a manifest with no experimental skills logs a nudge +>>> [CLI] experimental aitools install --global --experimental +Command "install" is deprecated, use "databricks aitools install" instead. +Installing Databricks AI skills for Claude Code... +Using skills version test-ref +Warn: --experimental was set but the manifest at test-ref exposes no experimental skills. Set DATABRICKS_SKILLS_REF to a release that includes them (or =main for the latest). +Installed 1 skill. diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/script b/acceptance/experimental/aitools/skills/install-experimental-empty/script new file mode 100644 index 00000000000..56fca80e389 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/script @@ -0,0 +1,4 @@ +mkdir -p "$HOME/.claude" + +title "--experimental against a manifest with no experimental skills logs a nudge" +trace $CLI experimental aitools install --global --experimental diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/test.toml b/acceptance/experimental/aitools/skills/install-experimental-empty/test.toml new file mode 100644 index 00000000000..e1bac3df588 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/test.toml @@ -0,0 +1,32 @@ +Env.DATABRICKS_SKILLS_BASE_URL = "$DATABRICKS_HOST" +Env.DATABRICKS_SKILLS_REF = "test-ref" + +Ignore = [ + ".databricks/aitools/skills/.state.json", +] + +[EnvMatrix] +DATABRICKS_BUNDLE_ENGINE = [] + +# Manifest with stable skills only — no entries with repo_dir=experimental. +# Simulates a release tag that pre-dates the experimental feature. +[[Server]] +Pattern = "GET /test-ref/manifest.json" +Response.Body = ''' +{ + "version": "2", + "updated_at": "2026-01-01T00:00:00Z", + "skills": { + "test-stable": {"version": "1.0.0", "files": ["SKILL.md"]} + } +} +''' + +[[Server]] +Pattern = "GET /test-ref/skills/test-stable/SKILL.md" +Response.Body = '''--- +name: test-stable +--- + +# Stable +''' diff --git a/acceptance/experimental/aitools/skills/install-specific/out.test.toml b/acceptance/experimental/aitools/skills/install-specific/out.test.toml new file mode 100644 index 00000000000..d6187dcb046 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-specific/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/experimental/aitools/skills/install-specific/output.txt b/acceptance/experimental/aitools/skills/install-specific/output.txt new file mode 100644 index 00000000000..e38470435b2 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-specific/output.txt @@ -0,0 +1,23 @@ + +=== install only one specific stable skill via --skills +>>> [CLI] experimental aitools install --global --skills test-stable-a +Command "install" is deprecated, use "databricks aitools install" instead. +Installing Databricks AI skills for Claude Code... +Using skills version test-ref +Installed 1 skill. + +=== install a specific experimental skill (note the -experimental suffix) +>>> [CLI] experimental aitools install --global --skills test-exp-experimental --experimental +Command "install" is deprecated, use "databricks aitools install" instead. +Installing Databricks AI skills for Claude Code... +Using skills version test-ref +Installed 1 skill. + +=== asking for an experimental skill without --experimental flag errors out +>>> [CLI] experimental aitools install --global --skills test-exp-experimental +Command "install" is deprecated, use "databricks aitools install" instead. +Installing Databricks AI skills for Claude Code... +Using skills version test-ref +Error: skill "test-exp-experimental" is experimental; use --experimental to install + +Exit code: 1 diff --git a/acceptance/experimental/aitools/skills/install-specific/script b/acceptance/experimental/aitools/skills/install-specific/script new file mode 100644 index 00000000000..f54391dd19a --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-specific/script @@ -0,0 +1,10 @@ +mkdir -p "$HOME/.claude" + +title "install only one specific stable skill via --skills" +trace $CLI experimental aitools install --global --skills test-stable-a + +title "install a specific experimental skill (note the -experimental suffix)" +trace $CLI experimental aitools install --global --skills test-exp-experimental --experimental + +title "asking for an experimental skill without --experimental flag errors out" +errcode trace $CLI experimental aitools install --global --skills test-exp-experimental diff --git a/acceptance/experimental/aitools/skills/install-specific/test.toml b/acceptance/experimental/aitools/skills/install-specific/test.toml new file mode 100644 index 00000000000..9be89c8b92a --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-specific/test.toml @@ -0,0 +1,50 @@ +Env.DATABRICKS_SKILLS_BASE_URL = "$DATABRICKS_HOST" +Env.DATABRICKS_SKILLS_REF = "test-ref" + +Ignore = [ + ".databricks/aitools/skills/.state.json", +] + +[EnvMatrix] +DATABRICKS_BUNDLE_ENGINE = [] + +[[Server]] +Pattern = "GET /test-ref/manifest.json" +Response.Body = ''' +{ + "version": "2", + "updated_at": "2026-01-01T00:00:00Z", + "skills": { + "test-stable-a": {"version": "1.0.0", "files": ["SKILL.md"], "repo_dir": "skills"}, + "test-stable-b": {"version": "1.0.0", "files": ["SKILL.md"], "repo_dir": "skills"}, + "test-exp": {"version": "0.0.1", "files": ["SKILL.md"], "repo_dir": "experimental"} + } +} +''' + +[[Server]] +Pattern = "GET /test-ref/skills/test-stable-a/SKILL.md" +Response.Body = '''--- +name: test-stable-a +--- + +# A +''' + +[[Server]] +Pattern = "GET /test-ref/skills/test-stable-b/SKILL.md" +Response.Body = '''--- +name: test-stable-b +--- + +# B +''' + +[[Server]] +Pattern = "GET /test-ref/experimental/test-exp/SKILL.md" +Response.Body = '''--- +name: test-exp +--- + +# Exp +''' diff --git a/acceptance/experimental/aitools/skills/install/out.test.toml b/acceptance/experimental/aitools/skills/install/out.test.toml new file mode 100644 index 00000000000..d6187dcb046 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/experimental/aitools/skills/install/output.txt b/acceptance/experimental/aitools/skills/install/output.txt new file mode 100644 index 00000000000..b33021d09b3 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install/output.txt @@ -0,0 +1,21 @@ + +=== stable-only install (no --experimental flag) installs 1 skill +>>> [CLI] experimental aitools install --global +Command "install" is deprecated, use "databricks aitools install" instead. +Installing Databricks AI skills for Claude Code... +Using skills version test-ref +Installed 1 skill. + +=== re-run with --experimental installs the experimental one too +>>> [CLI] experimental aitools install --global --experimental +Command "install" is deprecated, use "databricks aitools install" instead. +Installing Databricks AI skills for Claude Code... +Using skills version test-ref +Installed 2 skills. + +=== no-op re-run is idempotent (no new fetches, no errors) +>>> [CLI] experimental aitools install --global --experimental +Command "install" is deprecated, use "databricks aitools install" instead. +Installing Databricks AI skills for Claude Code... +Using skills version test-ref +Installed 2 skills. diff --git a/acceptance/experimental/aitools/skills/install/script b/acceptance/experimental/aitools/skills/install/script new file mode 100644 index 00000000000..616e4b22c6d --- /dev/null +++ b/acceptance/experimental/aitools/skills/install/script @@ -0,0 +1,11 @@ +# Fake a Claude Code installation so agent detection picks it up. +mkdir -p "$HOME/.claude" + +title "stable-only install (no --experimental flag) installs 1 skill" +trace $CLI experimental aitools install --global + +title "re-run with --experimental installs the experimental one too" +trace $CLI experimental aitools install --global --experimental + +title "no-op re-run is idempotent (no new fetches, no errors)" +trace $CLI experimental aitools install --global --experimental diff --git a/acceptance/experimental/aitools/skills/install/test.toml b/acceptance/experimental/aitools/skills/install/test.toml new file mode 100644 index 00000000000..5995a055eaf --- /dev/null +++ b/acceptance/experimental/aitools/skills/install/test.toml @@ -0,0 +1,59 @@ +# Point the manifest + skill-file fetch at the acceptance mock server +# instead of raw.githubusercontent.com. +Env.DATABRICKS_SKILLS_BASE_URL = "$DATABRICKS_HOST" +Env.DATABRICKS_SKILLS_REF = "test-ref" + +# The state file mtime + path under $HOME contain timestamps and the +# random temp dir name; squash those from the captured tree. +Ignore = [ + ".databricks/aitools/skills/.state.json", +] + +# aitools doesn't use the bundle engine; opt out of the parent matrix. +[EnvMatrix] +DATABRICKS_BUNDLE_ENGINE = [] + +# Mock the manifest. One stable skill, one experimental skill — the +# experimental state is conveyed by the per-skill repo_dir field, not a +# separate experimental_skills map. +[[Server]] +Pattern = "GET /test-ref/manifest.json" +Response.Body = ''' +{ + "version": "2", + "updated_at": "2026-01-01T00:00:00Z", + "skills": { + "test-stable": { + "version": "1.0.0", + "description": "Stable test skill", + "files": ["SKILL.md"], + "repo_dir": "skills" + }, + "test-exp": { + "version": "0.0.1", + "description": "Experimental test skill", + "files": ["SKILL.md"], + "repo_dir": "experimental" + } + } +} +''' + +# Mock the per-skill file fetches. +[[Server]] +Pattern = "GET /test-ref/skills/test-stable/SKILL.md" +Response.Body = '''--- +name: test-stable +--- + +# Test stable skill +''' + +[[Server]] +Pattern = "GET /test-ref/experimental/test-exp/SKILL.md" +Response.Body = '''--- +name: test-exp +--- + +# Test experimental skill +''' diff --git a/cmd/aitools/list.go b/cmd/aitools/list.go index bb9a01b3bd2..b0b3e877a80 100644 --- a/cmd/aitools/list.go +++ b/cmd/aitools/list.go @@ -102,7 +102,7 @@ func defaultListSkills(cmd *cobra.Command, scope string) error { meta := manifest.Skills[name] tag := "" - if meta.Experimental { + if meta.IsExperimental() { tag = " [experimental]" } diff --git a/libs/aitools/installer/installer.go b/libs/aitools/installer/installer.go index b905202d4c5..f26640ae192 100644 --- a/libs/aitools/installer/installer.go +++ b/libs/aitools/installer/installer.go @@ -24,14 +24,40 @@ import ( ) const ( - skillsRepoOwner = "databricks" - skillsRepoName = "databricks-agent-skills" - skillsRepoPath = "skills" + skillsRepoOwner = "databricks" + skillsRepoName = "databricks-agent-skills" + stableSkillsRepoPath = "skills" + experimentalRepoPath = "experimental" + experimentalSuffix = "-experimental" ) +// manifestHasExperimental reports whether the manifest contains at least one +// experimental skill (sourced from the experimental/ directory upstream). +func manifestHasExperimental(m *Manifest) bool { + for _, meta := range m.Skills { + if meta.IsExperimental() { + return true + } + } + return false +} + +// alternateVariantKey returns the manifest key of the same logical skill +// under the opposite experimental status. For "databricks-jobs" it returns +// "databricks-jobs-experimental"; for "databricks-jobs-experimental" it +// returns "databricks-jobs". Used to clean up the previously-installed +// variant when a skill transitions between experimental and stable +// upstream. +func alternateVariantKey(key string) string { + if strings.HasSuffix(key, experimentalSuffix) { + return strings.TrimSuffix(key, experimentalSuffix) + } + return key + experimentalSuffix +} + // 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 +var fetchFileFn func(ctx context.Context, ref, repoDir, skillName, filePath string) ([]byte, error) = fetchSkillFile // GetSkillsRef returns the skills repo ref to use and whether it was explicitly // set via DATABRICKS_SKILLS_REF (as opposed to auto-resolved from the manifest). @@ -47,7 +73,23 @@ func GetSkillsRef(ctx context.Context) (ref string, explicit bool, err error) { return "v" + v, false, nil } +// GetSkillsBaseURL returns the base URL for fetching the manifest and skill +// files. If DATABRICKS_SKILLS_BASE_URL is set, it returns that; otherwise the +// canonical raw.githubusercontent.com URL for the upstream repo. The override +// is used by acceptance tests to point fetches at a mock HTTP server. +func GetSkillsBaseURL(ctx context.Context) string { + if base := env.Get(ctx, "DATABRICKS_SKILLS_BASE_URL"); base != "" { + return strings.TrimRight(base, "/") + } + return "https://raw.githubusercontent.com/" + skillsRepoOwner + "/" + skillsRepoName +} + // Manifest describes the skills manifest fetched from the skills repo. +// +// The repo exposes stable skills under skills/ and experimental skills under +// experimental/. Both appear in a single Skills map; each entry carries +// RepoDir indicating which top-level directory holds its files, which is +// also the source of truth for whether the skill is experimental. type Manifest struct { Version string `json:"version"` UpdatedAt string `json:"updated_at"` @@ -56,12 +98,31 @@ type Manifest struct { // SkillMeta describes a single skill entry in the manifest. type SkillMeta struct { - Version string `json:"version"` - UpdatedAt string `json:"updated_at"` - Files []string `json:"files"` - Experimental bool `json:"experimental,omitempty"` - Description string `json:"description,omitempty"` - MinCLIVer string `json:"min_cli_version,omitempty"` + Version string `json:"version"` + UpdatedAt string `json:"updated_at"` + Files []string `json:"files"` + Description string `json:"description,omitempty"` + MinCLIVer string `json:"min_cli_version,omitempty"` + + // RepoDir is the top-level repo subdirectory (skills/ or experimental/) + // that holds this skill's files. Provided by the manifest; serves as + // the source of truth for whether the skill is experimental. + RepoDir string `json:"repo_dir,omitempty"` + + // SourceName is the directory name within RepoDir that holds this + // skill's files in the upstream repo. For stable skills this equals + // the manifest key. For experimental skills the manifest key has a + // "-experimental" suffix appended for collision-free install paths, + // but the upstream repo dir does not — SourceName preserves it for + // the fetch URL. Populated during normalization; not part of the wire + // format. + SourceName string `json:"-"` +} + +// IsExperimental reports whether the skill is sourced from the experimental/ +// directory of the upstream repo. +func (s SkillMeta) IsExperimental() bool { + return s.RepoDir == experimentalRepoPath } // InstallOptions controls the behavior of InstallSkillsForAgents. @@ -71,9 +132,12 @@ type InstallOptions struct { Scope string // ScopeGlobal or ScopeProject (default: global) } -func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byte, error) { - url := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s/%s/%s", - skillsRepoOwner, skillsRepoName, ref, skillsRepoPath, skillName, filePath) +func fetchSkillFile(ctx context.Context, ref, repoDir, skillName, filePath string) ([]byte, error) { + if repoDir == "" { + repoDir = stableSkillsRepoPath + } + url := fmt.Sprintf("%s/%s/%s/%s/%s", + GetSkillsBaseURL(ctx), ref, repoDir, skillName, filePath) req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { @@ -128,6 +192,12 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent return err } + // Helpful nudge for users testing --experimental against a ref that + // pre-dates the experimental/ directory upstream. + if opts.IncludeExperimental && !manifestHasExperimental(manifest) { + log.Warnf(ctx, "--experimental was set but the manifest at %s exposes no experimental skills. Set DATABRICKS_SKILLS_REF to a release that includes them (or =main for the latest).", ref) + } + scope := opts.Scope if scope == "" { scope = ScopeGlobal @@ -186,6 +256,23 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent for _, name := range skillNames { meta := targetSkills[name] + + // Experimental↔stable transition: if the alternate variant of this + // skill was previously installed (upstream flipped its experimental + // status), remove the stale variant before installing the new one. + if state != nil { + alt := alternateVariantKey(name) + if _, ok := state.Skills[alt]; ok { + altDir := filepath.Join(baseDir, alt) + removeSymlinksFromAgents(ctx, alt, altDir, scope, cwd) + if err := os.RemoveAll(altDir); err != nil { + log.Warnf(ctx, "Failed to remove previous variant %s: %v", altDir, err) + } + delete(state.Skills, alt) + cmdio.LogString(ctx, fmt.Sprintf("Replaced previous variant %s with %s", alt, name)) + } + } + // Idempotency: skip if same version is already installed, the canonical // dir exists, AND every requested agent already has the skill on disk. if state != nil && state.Skills[name] == meta.Version { @@ -196,7 +283,7 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent } } - if err := installSkillForAgents(ctx, name, meta.Files, targetAgents, params); err != nil { + if err := installSkillForAgents(ctx, name, meta, targetAgents, params); err != nil { return err } } @@ -287,7 +374,7 @@ func resolveSkills(ctx context.Context, skills map[string]SkillMeta, opts Instal result := make(map[string]SkillMeta, len(candidates)) for name, meta := range candidates { - if meta.Experimental && !opts.IncludeExperimental { + if meta.IsExperimental() && !opts.IncludeExperimental { if isSpecific { return nil, fmt.Errorf("skill %q is experimental; use --experimental to install", name) } @@ -394,9 +481,9 @@ type installParams struct { ref string } -func installSkillForAgents(ctx context.Context, skillName string, files []string, detectedAgents []*agents.Agent, params installParams) error { +func installSkillForAgents(ctx context.Context, skillName string, meta SkillMeta, detectedAgents []*agents.Agent, params installParams) error { canonicalDir := filepath.Join(params.baseDir, skillName) - if err := installSkillToDir(ctx, params.ref, skillName, canonicalDir, files); err != nil { + if err := installSkillToDir(ctx, params.ref, meta.RepoDir, meta.SourceName, canonicalDir, meta.Files); err != nil { return err } @@ -494,7 +581,7 @@ func backupThirdPartySkill(ctx context.Context, destDir, canonicalDir, skillName return nil } -func installSkillToDir(ctx context.Context, ref, skillName, destDir string, files []string) error { +func installSkillToDir(ctx context.Context, ref, repoDir, skillName, destDir string, files []string) error { // remove existing skill directory for clean install if err := os.RemoveAll(destDir); err != nil { return fmt.Errorf("failed to remove existing skill: %w", err) @@ -505,7 +592,7 @@ func installSkillToDir(ctx context.Context, ref, skillName, destDir string, file } for _, file := range files { - content, err := fetchFileFn(ctx, ref, skillName, file) + content, err := fetchFileFn(ctx, ref, repoDir, skillName, file) if err != nil { return err } diff --git a/libs/aitools/installer/installer_test.go b/libs/aitools/installer/installer_test.go index 3ace56e952f..36e818b9926 100644 --- a/libs/aitools/installer/installer_test.go +++ b/libs/aitools/installer/installer_test.go @@ -29,6 +29,7 @@ func (m *mockManifestSource) FetchManifest(_ context.Context, _ string) (*Manife if m.fetchErr != nil { return nil, m.fetchErr } + normalizeManifest(m.manifest) return m.manifest, nil } @@ -53,7 +54,7 @@ func setupFetchMock(t *testing.T) { t.Helper() orig := fetchFileFn t.Cleanup(func() { fetchFileFn = orig }) - fetchFileFn = func(_ context.Context, _, skillName, filePath string) ([]byte, error) { + fetchFileFn = func(_ context.Context, _, _, skillName, filePath string) ([]byte, error) { return []byte("# " + skillName + "/" + filePath), nil } } @@ -261,10 +262,10 @@ func TestExperimentalSkillsSkippedByDefault(t *testing.T) { t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) manifest := testManifest() - manifest.Skills["databricks-experimental"] = SkillMeta{ - Version: "0.1.0", - Files: []string{"SKILL.md"}, - Experimental: true, + manifest.Skills["databricks-iceberg"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + RepoDir: experimentalRepoPath, } src := &mockManifestSource{manifest: manifest} @@ -278,7 +279,7 @@ func TestExperimentalSkillsSkippedByDefault(t *testing.T) { require.NoError(t, err) // Only non-experimental skills should be installed. assert.Len(t, state.Skills, 2) - assert.NotContains(t, state.Skills, "databricks-experimental") + assert.NotContains(t, state.Skills, "databricks-iceberg-experimental") assert.Contains(t, stderr.String(), "Installed 2 skills.") } @@ -290,10 +291,10 @@ func TestExperimentalSkillsIncludedWithFlag(t *testing.T) { t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) manifest := testManifest() - manifest.Skills["databricks-experimental"] = SkillMeta{ - Version: "0.1.0", - Files: []string{"SKILL.md"}, - Experimental: true, + manifest.Skills["databricks-iceberg"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + RepoDir: experimentalRepoPath, } src := &mockManifestSource{manifest: manifest} @@ -308,7 +309,7 @@ func TestExperimentalSkillsIncludedWithFlag(t *testing.T) { state, err := LoadState(globalDir) require.NoError(t, err) assert.Len(t, state.Skills, 3) - assert.Contains(t, state.Skills, "databricks-experimental") + assert.Contains(t, state.Skills, "databricks-iceberg-experimental") assert.True(t, state.IncludeExperimental) assert.Contains(t, stderr.String(), "Installed 3 skills.") @@ -392,7 +393,7 @@ func TestIdempotentSecondInstallSkips(t *testing.T) { fetchCalls := 0 orig := fetchFileFn t.Cleanup(func() { fetchFileFn = orig }) - fetchFileFn = func(_ context.Context, _, skillName, filePath string) ([]byte, error) { + fetchFileFn = func(_ context.Context, _, _, skillName, filePath string) ([]byte, error) { fetchCalls++ return []byte("# " + skillName + "/" + filePath), nil } @@ -430,7 +431,7 @@ func TestIdempotentInstallUpdatesNewVersions(t *testing.T) { var fetchedSkills []string orig := fetchFileFn t.Cleanup(func() { fetchFileFn = orig }) - fetchFileFn = func(_ context.Context, _, skillName, filePath string) ([]byte, error) { + fetchFileFn = func(_ context.Context, _, _, skillName, filePath string) ([]byte, error) { fetchedSkills = append(fetchedSkills, skillName) return []byte("# " + skillName + "/" + filePath), nil } @@ -517,7 +518,7 @@ func TestIdempotentInstallReinstallsForNewAgent(t *testing.T) { fetchCalls := 0 orig := fetchFileFn t.Cleanup(func() { fetchFileFn = orig }) - fetchFileFn = func(_ context.Context, _, skillName, filePath string) ([]byte, error) { + fetchFileFn = func(_ context.Context, _, _, skillName, filePath string) ([]byte, error) { fetchCalls++ return []byte("# " + skillName + "/" + filePath), nil } @@ -727,6 +728,51 @@ func TestInstallProjectScopeZeroCompatibleAgentsReturnsError(t *testing.T) { assert.Contains(t, err.Error(), "No Project Agent") } +func TestInstallReplacesAlternateVariant(t *testing.T) { + // Setup: a skill called "databricks-jobs" is installed as stable. + // Then the manifest re-categorizes it as experimental (key becomes + // "databricks-jobs-experimental"). A new install with --experimental + // should remove the stale stable variant and install the experimental one. + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + agent := testAgent(tmp) + + stableManifest := &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}}, + }, + } + require.NoError(t, InstallSkillsForAgents( + ctx, &mockManifestSource{manifest: stableManifest}, + []*agents.Agent{agent}, InstallOptions{}, + )) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + require.DirExists(t, filepath.Join(globalDir, "databricks-jobs")) + + // Now flip to experimental upstream. New install run. + experimentalManifest := &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.2.0", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, + }, + } + require.NoError(t, InstallSkillsForAgents( + ctx, &mockManifestSource{manifest: experimentalManifest}, + []*agents.Agent{agent}, InstallOptions{IncludeExperimental: true}, + )) + + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.NotContains(t, state.Skills, "databricks-jobs", "stale stable variant should be removed from state") + assert.Equal(t, "0.2.0", state.Skills["databricks-jobs-experimental"]) + assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs"), "stale stable install dir should be gone") + assert.DirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) + assert.Contains(t, stderr.String(), "Replaced previous variant databricks-jobs with databricks-jobs-experimental") +} + func TestSupportsProjectScopeSetCorrectly(t *testing.T) { expected := map[string]bool{ "claude-code": true, diff --git a/libs/aitools/installer/source.go b/libs/aitools/installer/source.go index 5dc78a254fd..6a7bf789a99 100644 --- a/libs/aitools/installer/source.go +++ b/libs/aitools/installer/source.go @@ -23,8 +23,7 @@ type GitHubManifestSource struct{} // FetchManifest fetches the skills manifest from GitHub at the given ref. func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (*Manifest, error) { log.Debugf(ctx, "Fetching skills manifest from %s/%s@%s", skillsRepoOwner, skillsRepoName, ref) - url := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/manifest.json", - skillsRepoOwner, skillsRepoName, ref) + url := fmt.Sprintf("%s/%s/manifest.json", GetSkillsBaseURL(ctx), ref) req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { @@ -50,5 +49,33 @@ func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (* return nil, fmt.Errorf("failed to parse manifest: %w", err) } + normalizeManifest(&manifest) return &manifest, nil } + +// normalizeManifest stamps SourceName, defaults a missing RepoDir to the +// stable directory, and re-keys experimental skills with a "-experimental" +// suffix so their on-disk install paths can't collide with stable skills +// of the same name. SourceName preserves the upstream repo directory name +// (which does not carry the suffix) for fetch URLs. +// +// RepoDir is provided by the manifest and is the source of truth for +// whether a skill is experimental — see SkillMeta.IsExperimental. +func normalizeManifest(m *Manifest) { + if m.Skills == nil { + m.Skills = map[string]SkillMeta{} + } + out := make(map[string]SkillMeta, len(m.Skills)) + for name, meta := range m.Skills { + if meta.RepoDir == "" { + meta.RepoDir = stableSkillsRepoPath + } + meta.SourceName = name + if meta.IsExperimental() { + out[name+experimentalSuffix] = meta + } else { + out[name] = meta + } + } + m.Skills = out +} diff --git a/libs/aitools/installer/source_test.go b/libs/aitools/installer/source_test.go new file mode 100644 index 00000000000..12b6aec9c22 --- /dev/null +++ b/libs/aitools/installer/source_test.go @@ -0,0 +1,73 @@ +package installer + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNormalizeManifestStampsSourceNameAndSuffixesExperimental(t *testing.T) { + m := &Manifest{ + Skills: map[string]SkillMeta{ + "databricks-apps": {Version: "0.1.0", Files: []string{"SKILL.md"}}, + "databricks-iceberg": {Version: "0.0.1", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, + }, + } + + normalizeManifest(m) + + stable := m.Skills["databricks-apps"] + assert.False(t, stable.IsExperimental()) + assert.Equal(t, stableSkillsRepoPath, stable.RepoDir, "missing RepoDir defaults to stable") + assert.Equal(t, "databricks-apps", stable.SourceName, "stable SourceName equals key") + + exp, ok := m.Skills["databricks-iceberg-experimental"] + assert.True(t, ok, "experimental skills always get -experimental suffix on the manifest key") + assert.True(t, exp.IsExperimental()) + assert.Equal(t, experimentalRepoPath, exp.RepoDir) + assert.Equal(t, "databricks-iceberg", exp.SourceName, "SourceName preserves the upstream repo dir name (no suffix)") + + _, original := m.Skills["databricks-iceberg"] + assert.False(t, original, "the un-suffixed key should not be present") +} + +func TestManifestHasExperimental(t *testing.T) { + stableOnly := &Manifest{Skills: map[string]SkillMeta{ + "databricks-apps": {Version: "0.1.0"}, + }} + normalizeManifest(stableOnly) + assert.False(t, manifestHasExperimental(stableOnly)) + + withExperimental := &Manifest{ + Skills: map[string]SkillMeta{ + "databricks-apps": {Version: "0.1.0"}, + "databricks-iceberg": {Version: "0.0.1", RepoDir: experimentalRepoPath}, + }, + } + normalizeManifest(withExperimental) + assert.True(t, manifestHasExperimental(withExperimental)) +} + +func TestAlternateVariantKey(t *testing.T) { + assert.Equal(t, "databricks-jobs-experimental", alternateVariantKey("databricks-jobs")) + assert.Equal(t, "databricks-jobs", alternateVariantKey("databricks-jobs-experimental")) + // Idempotent under double application + assert.Equal(t, "databricks-jobs", + alternateVariantKey(alternateVariantKey("databricks-jobs"))) +} + +func TestNormalizeManifestOnlyExperimentalSkills(t *testing.T) { + m := &Manifest{ + Skills: map[string]SkillMeta{ + "x": {Version: "0.0.1", RepoDir: experimentalRepoPath}, + }, + } + + normalizeManifest(m) + + got, ok := m.Skills["x-experimental"] + assert.True(t, ok) + assert.True(t, got.IsExperimental()) + assert.Equal(t, experimentalRepoPath, got.RepoDir) + assert.Equal(t, "x", got.SourceName) +} diff --git a/libs/aitools/installer/uninstall.go b/libs/aitools/installer/uninstall.go index da4cc6378c0..4236f6ff868 100644 --- a/libs/aitools/installer/uninstall.go +++ b/libs/aitools/installer/uninstall.go @@ -63,15 +63,30 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { var toRemove []string if len(opts.Skills) > 0 { seen := make(map[string]bool) - for _, name := range opts.Skills { + for _, raw := range opts.Skills { + // Accept either variant: if the literal name isn't installed but + // the alternate variant is, use the alternate. This makes uninstall + // resilient to the experimental↔stable transition. + name := raw + if _, ok := state.Skills[name]; !ok { + if alt := alternateVariantKey(name); state.Skills[alt] != "" { + name = alt + } else { + return fmt.Errorf("skill %q is not installed", raw) + } + } if seen[name] { continue } seen[name] = true - if _, ok := state.Skills[name]; !ok { - return fmt.Errorf("skill %q is not installed", name) - } toRemove = append(toRemove, name) + + // If both variants are installed, remove both (the alternate is + // the stale "old version" of the same logical skill). + if alt := alternateVariantKey(name); state.Skills[alt] != "" && !seen[alt] { + seen[alt] = true + toRemove = append(toRemove, alt) + } } } else { for name := range state.Skills { diff --git a/libs/aitools/installer/uninstall_test.go b/libs/aitools/installer/uninstall_test.go index 3dc8e03af35..8f2c8906568 100644 --- a/libs/aitools/installer/uninstall_test.go +++ b/libs/aitools/installer/uninstall_test.go @@ -322,6 +322,81 @@ func TestUninstallSelectiveDuplicateNamesDeduplicates(t *testing.T) { assert.Contains(t, stderr.String(), "Uninstalled 1 skill.") } +func TestUninstallByEitherVariantRemovesBoth(t *testing.T) { + // Setup: both stable and experimental variants of databricks-jobs are + // installed (the transitional state we want uninstall to clean up). + // The new manifest format only carries one entry per logical skill, so + // the both-installed state is constructed here by installing the stable + // variant via the manifest and seeding the experimental variant directly + // on disk + in state — simulating an install left over from a prior ref + // where the same skill lived under experimental/. + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + agent := testAgent(tmp) + + stableManifest := &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}}, + }, + } + require.NoError(t, InstallSkillsForAgents( + ctx, &mockManifestSource{manifest: stableManifest}, + []*agents.Agent{agent}, InstallOptions{}, + )) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + require.DirExists(t, filepath.Join(globalDir, "databricks-jobs")) + + // Seed the leftover experimental variant. + expDir := filepath.Join(globalDir, "databricks-jobs-experimental") + require.NoError(t, os.MkdirAll(expDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(expDir, "SKILL.md"), []byte("# exp"), 0o644)) + state, err := LoadState(globalDir) + require.NoError(t, err) + state.Skills["databricks-jobs-experimental"] = "0.0.1" + require.NoError(t, SaveState(globalDir, state)) + + // Uninstall using just the un-suffixed name; both variants should go. + require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{ + Skills: []string{"databricks-jobs"}, + })) + + assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs")) + assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) + assert.Contains(t, stderr.String(), "Uninstalled 2 skills.") +} + +func TestUninstallByAlternateNameWhenOnlyOneVariantInstalled(t *testing.T) { + // Setup: only the experimental variant is installed. + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + agent := testAgent(tmp) + + manifest := &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.0.1", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, + }, + } + require.NoError(t, InstallSkillsForAgents( + ctx, &mockManifestSource{manifest: manifest}, + []*agents.Agent{agent}, InstallOptions{IncludeExperimental: true}, + )) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + require.DirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) + + // User types the un-suffixed name (doesn't know which variant is on + // disk); uninstall should still find and remove it. + require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{ + Skills: []string{"databricks-jobs"}, + })) + assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) +} + func TestUninstallSelectiveAllRemovesStateFile(t *testing.T) { tmp := setupTestHome(t) globalDir := installTestSkills(t, tmp) diff --git a/libs/aitools/installer/update.go b/libs/aitools/installer/update.go index 9965ff7fc63..d1e8eecc4c2 100644 --- a/libs/aitools/installer/update.go +++ b/libs/aitools/installer/update.go @@ -125,7 +125,7 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent } // Filter experimental skills unless state opted in. - if meta.Experimental && !state.IncludeExperimental { + if meta.IsExperimental() && !state.IncludeExperimental { log.Debugf(ctx, "Skipping experimental skill %s", name) result.Skipped = append(result.Skipped, name) continue @@ -177,7 +177,7 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent for _, change := range allChanges { meta := manifest.Skills[change.Name] - if err := installSkillForAgents(ctx, change.Name, meta.Files, targetAgents, params); err != nil { + if err := installSkillForAgents(ctx, change.Name, meta, targetAgents, params); err != nil { return nil, err } } diff --git a/libs/aitools/installer/update_test.go b/libs/aitools/installer/update_test.go index a82be1d0289..e4b8d1d5953 100644 --- a/libs/aitools/installer/update_test.go +++ b/libs/aitools/installer/update_test.go @@ -132,7 +132,7 @@ func TestUpdateCheckDryRun(t *testing.T) { fetchCalls := 0 orig := fetchFileFn t.Cleanup(func() { fetchFileFn = orig }) - fetchFileFn = func(_ context.Context, _, _, _ string) ([]byte, error) { + fetchFileFn = func(_ context.Context, _, _, _, _ string) ([]byte, error) { fetchCalls++ return []byte("content"), nil } @@ -169,7 +169,7 @@ func TestUpdateForceRedownloads(t *testing.T) { fetchCalls := 0 orig := fetchFileFn t.Cleanup(func() { fetchFileFn = orig }) - fetchFileFn = func(_ context.Context, _, _, _ string) ([]byte, error) { + fetchFileFn = func(_ context.Context, _, _, _, _ string) ([]byte, error) { fetchCalls++ return []byte("content"), nil } @@ -345,10 +345,10 @@ func TestUpdateSkipsExperimentalSkills(t *testing.T) { // New manifest with an experimental skill. updatedManifest := testManifest() - updatedManifest.Skills["databricks-experimental"] = SkillMeta{ - Version: "0.1.0", - Files: []string{"SKILL.md"}, - Experimental: true, + updatedManifest.Skills["databricks-iceberg"] = SkillMeta{ + Version: "0.1.0", + Files: []string{"SKILL.md"}, + RepoDir: experimentalRepoPath, } src2 := &mockManifestSource{manifest: updatedManifest} @@ -356,7 +356,7 @@ func TestUpdateSkipsExperimentalSkills(t *testing.T) { require.NoError(t, err) // Experimental skill should be skipped. - assert.Contains(t, result.Skipped, "databricks-experimental") + assert.Contains(t, result.Skipped, "databricks-iceberg-experimental") assert.Empty(t, result.Added) } From eac372c51584137f88aace7fcb896f9f68e35fc3 Mon Sep 17 00:00:00 2001 From: James Broadhead Date: Tue, 19 May 2026 14:56:45 +0000 Subject: [PATCH 2/5] aitools: lint nits + Windows-safe acceptance script (re-apply) Re-applied after the latest squash-and-rebase reset HEAD: - libs/aitools/installer/installer.go: alternateVariantKey uses strings.CutSuffix (modernize lint hint that now hard-fails CI). - libs/aitools/installer/uninstall.go: drop the redundant `name := raw` copy of the loop variable (copyloopvar lint hint; Go 1.22+ semantics). - acceptance/experimental/aitools/skills/*/script: stub the Claude Code install dir under "${USERPROFILE:-$HOME}/.claude". On Windows the acceptance harness sets USERPROFILE (Go's os.UserHomeDir backing var on that platform), not HOME, so the previous `$HOME/.claude` wrote to the wrong path and the cli reported "No supported coding agents detected." The fallback keeps Linux/macOS behavior unchanged. Co-authored-by: Isaac --- .../aitools/skills/install-experimental-empty/script | 6 +++++- .../experimental/aitools/skills/install-specific/script | 6 +++++- acceptance/experimental/aitools/skills/install/script | 7 +++++-- libs/aitools/installer/installer.go | 4 ++-- libs/aitools/installer/uninstall.go | 5 ++--- 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/script b/acceptance/experimental/aitools/skills/install-experimental-empty/script index 56fca80e389..0790d9b7c54 100644 --- a/acceptance/experimental/aitools/skills/install-experimental-empty/script +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/script @@ -1,4 +1,8 @@ -mkdir -p "$HOME/.claude" +# Stub a Claude Code install under the harness-set home dir so agent +# detection picks it up. On Windows the harness sets USERPROFILE (Go's +# os.UserHomeDir backing var on that platform), not HOME — use USERPROFILE +# when set, fall back to HOME for Linux/macOS. +mkdir -p "${USERPROFILE:-$HOME}/.claude" title "--experimental against a manifest with no experimental skills logs a nudge" trace $CLI experimental aitools install --global --experimental diff --git a/acceptance/experimental/aitools/skills/install-specific/script b/acceptance/experimental/aitools/skills/install-specific/script index f54391dd19a..e68e4e199b2 100644 --- a/acceptance/experimental/aitools/skills/install-specific/script +++ b/acceptance/experimental/aitools/skills/install-specific/script @@ -1,4 +1,8 @@ -mkdir -p "$HOME/.claude" +# Stub a Claude Code install under the harness-set home dir so agent +# detection picks it up. On Windows the harness sets USERPROFILE (Go's +# os.UserHomeDir backing var on that platform), not HOME — use USERPROFILE +# when set, fall back to HOME for Linux/macOS. +mkdir -p "${USERPROFILE:-$HOME}/.claude" title "install only one specific stable skill via --skills" trace $CLI experimental aitools install --global --skills test-stable-a diff --git a/acceptance/experimental/aitools/skills/install/script b/acceptance/experimental/aitools/skills/install/script index 616e4b22c6d..951b9e3cabd 100644 --- a/acceptance/experimental/aitools/skills/install/script +++ b/acceptance/experimental/aitools/skills/install/script @@ -1,5 +1,8 @@ -# Fake a Claude Code installation so agent detection picks it up. -mkdir -p "$HOME/.claude" +# Stub a Claude Code install under the harness-set home dir so agent +# detection picks it up. On Windows the harness sets USERPROFILE (Go's +# os.UserHomeDir backing var on that platform), not HOME — use USERPROFILE +# when set, fall back to HOME for Linux/macOS. +mkdir -p "${USERPROFILE:-$HOME}/.claude" title "stable-only install (no --experimental flag) installs 1 skill" trace $CLI experimental aitools install --global diff --git a/libs/aitools/installer/installer.go b/libs/aitools/installer/installer.go index f26640ae192..9abe5fd850d 100644 --- a/libs/aitools/installer/installer.go +++ b/libs/aitools/installer/installer.go @@ -49,8 +49,8 @@ func manifestHasExperimental(m *Manifest) bool { // variant when a skill transitions between experimental and stable // upstream. func alternateVariantKey(key string) string { - if strings.HasSuffix(key, experimentalSuffix) { - return strings.TrimSuffix(key, experimentalSuffix) + if base, ok := strings.CutSuffix(key, experimentalSuffix); ok { + return base } return key + experimentalSuffix } diff --git a/libs/aitools/installer/uninstall.go b/libs/aitools/installer/uninstall.go index 4236f6ff868..821f49d6a29 100644 --- a/libs/aitools/installer/uninstall.go +++ b/libs/aitools/installer/uninstall.go @@ -63,16 +63,15 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { var toRemove []string if len(opts.Skills) > 0 { seen := make(map[string]bool) - for _, raw := range opts.Skills { + for _, name := range opts.Skills { // Accept either variant: if the literal name isn't installed but // the alternate variant is, use the alternate. This makes uninstall // resilient to the experimental↔stable transition. - name := raw if _, ok := state.Skills[name]; !ok { if alt := alternateVariantKey(name); state.Skills[alt] != "" { name = alt } else { - return fmt.Errorf("skill %q is not installed", raw) + return fmt.Errorf("skill %q is not installed", name) } } if seen[name] { From fc7f6162e68792e36635b32ea96c22879e3fadea Mon Sep 17 00:00:00 2001 From: simon Date: Wed, 20 May 2026 16:47:56 +0200 Subject: [PATCH 3/5] aitools: handle experimental variant updates Ensure update cleans up stale stable/experimental skill variants and keep the manifest comments focused on the invariants that matter. --- .../skills/install-experimental-empty/script | 5 +- .../install-experimental-empty/test.toml | 3 +- .../aitools/skills/install-specific/script | 5 +- .../aitools/skills/install/script | 5 +- .../aitools/skills/install/test.toml | 11 +- libs/aitools/installer/installer.go | 83 +++++++------- libs/aitools/installer/installer_test.go | 9 +- libs/aitools/installer/source.go | 10 +- libs/aitools/installer/source_test.go | 9 +- libs/aitools/installer/uninstall.go | 16 ++- libs/aitools/installer/uninstall_test.go | 13 +-- libs/aitools/installer/update.go | 23 ++-- libs/aitools/installer/update_test.go | 101 ++++++++++++++++++ 13 files changed, 172 insertions(+), 121 deletions(-) diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/script b/acceptance/experimental/aitools/skills/install-experimental-empty/script index 0790d9b7c54..cf27462115c 100644 --- a/acceptance/experimental/aitools/skills/install-experimental-empty/script +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/script @@ -1,7 +1,4 @@ -# Stub a Claude Code install under the harness-set home dir so agent -# detection picks it up. On Windows the harness sets USERPROFILE (Go's -# os.UserHomeDir backing var on that platform), not HOME — use USERPROFILE -# when set, fall back to HOME for Linux/macOS. +# Agent detection needs ~/.claude; prefer USERPROFILE on Windows. mkdir -p "${USERPROFILE:-$HOME}/.claude" title "--experimental against a manifest with no experimental skills logs a nudge" diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/test.toml b/acceptance/experimental/aitools/skills/install-experimental-empty/test.toml index e1bac3df588..0e77cc5247d 100644 --- a/acceptance/experimental/aitools/skills/install-experimental-empty/test.toml +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/test.toml @@ -8,8 +8,7 @@ Ignore = [ [EnvMatrix] DATABRICKS_BUNDLE_ENGINE = [] -# Manifest with stable skills only — no entries with repo_dir=experimental. -# Simulates a release tag that pre-dates the experimental feature. +# Stable-only manifest (no repo_dir=experimental). [[Server]] Pattern = "GET /test-ref/manifest.json" Response.Body = ''' diff --git a/acceptance/experimental/aitools/skills/install-specific/script b/acceptance/experimental/aitools/skills/install-specific/script index e68e4e199b2..e6eac21d461 100644 --- a/acceptance/experimental/aitools/skills/install-specific/script +++ b/acceptance/experimental/aitools/skills/install-specific/script @@ -1,7 +1,4 @@ -# Stub a Claude Code install under the harness-set home dir so agent -# detection picks it up. On Windows the harness sets USERPROFILE (Go's -# os.UserHomeDir backing var on that platform), not HOME — use USERPROFILE -# when set, fall back to HOME for Linux/macOS. +# Agent detection needs ~/.claude; prefer USERPROFILE on Windows. mkdir -p "${USERPROFILE:-$HOME}/.claude" title "install only one specific stable skill via --skills" diff --git a/acceptance/experimental/aitools/skills/install/script b/acceptance/experimental/aitools/skills/install/script index 951b9e3cabd..cc64fb54951 100644 --- a/acceptance/experimental/aitools/skills/install/script +++ b/acceptance/experimental/aitools/skills/install/script @@ -1,7 +1,4 @@ -# Stub a Claude Code install under the harness-set home dir so agent -# detection picks it up. On Windows the harness sets USERPROFILE (Go's -# os.UserHomeDir backing var on that platform), not HOME — use USERPROFILE -# when set, fall back to HOME for Linux/macOS. +# Agent detection needs ~/.claude; prefer USERPROFILE on Windows. mkdir -p "${USERPROFILE:-$HOME}/.claude" title "stable-only install (no --experimental flag) installs 1 skill" diff --git a/acceptance/experimental/aitools/skills/install/test.toml b/acceptance/experimental/aitools/skills/install/test.toml index 5995a055eaf..f21efd9189a 100644 --- a/acceptance/experimental/aitools/skills/install/test.toml +++ b/acceptance/experimental/aitools/skills/install/test.toml @@ -1,21 +1,15 @@ -# Point the manifest + skill-file fetch at the acceptance mock server -# instead of raw.githubusercontent.com. +# Mock server replaces raw.githubusercontent.com for manifest + skill files. Env.DATABRICKS_SKILLS_BASE_URL = "$DATABRICKS_HOST" Env.DATABRICKS_SKILLS_REF = "test-ref" -# The state file mtime + path under $HOME contain timestamps and the -# random temp dir name; squash those from the captured tree. Ignore = [ ".databricks/aitools/skills/.state.json", ] -# aitools doesn't use the bundle engine; opt out of the parent matrix. [EnvMatrix] DATABRICKS_BUNDLE_ENGINE = [] -# Mock the manifest. One stable skill, one experimental skill — the -# experimental state is conveyed by the per-skill repo_dir field, not a -# separate experimental_skills map. +# Stable + experimental skills via repo_dir. [[Server]] Pattern = "GET /test-ref/manifest.json" Response.Body = ''' @@ -39,7 +33,6 @@ Response.Body = ''' } ''' -# Mock the per-skill file fetches. [[Server]] Pattern = "GET /test-ref/skills/test-stable/SKILL.md" Response.Body = '''--- diff --git a/libs/aitools/installer/installer.go b/libs/aitools/installer/installer.go index 9abe5fd850d..b454e5a8aea 100644 --- a/libs/aitools/installer/installer.go +++ b/libs/aitools/installer/installer.go @@ -31,8 +31,6 @@ const ( experimentalSuffix = "-experimental" ) -// manifestHasExperimental reports whether the manifest contains at least one -// experimental skill (sourced from the experimental/ directory upstream). func manifestHasExperimental(m *Manifest) bool { for _, meta := range m.Skills { if meta.IsExperimental() { @@ -42,12 +40,7 @@ func manifestHasExperimental(m *Manifest) bool { return false } -// alternateVariantKey returns the manifest key of the same logical skill -// under the opposite experimental status. For "databricks-jobs" it returns -// "databricks-jobs-experimental"; for "databricks-jobs-experimental" it -// returns "databricks-jobs". Used to clean up the previously-installed -// variant when a skill transitions between experimental and stable -// upstream. +// alternateVariantKey maps between "foo" and "foo-experimental". func alternateVariantKey(key string) string { if base, ok := strings.CutSuffix(key, experimentalSuffix); ok { return base @@ -55,6 +48,35 @@ func alternateVariantKey(key string) string { return key + experimentalSuffix } +// installedSkillVersion returns the recorded version for name, or for its +// stable/experimental alternate when only that variant is installed. +func installedSkillVersion(state *InstallState, name string) (version string, selfInstalled, alternateInstalled bool) { + version, selfInstalled = state.Skills[name] + altVersion, alternateInstalled := state.Skills[alternateVariantKey(name)] + if !selfInstalled && alternateInstalled { + version = altVersion + } + return version, selfInstalled, alternateInstalled +} + +func removeAlternateVariant(ctx context.Context, state *InstallState, baseDir, name, scope, cwd string) { + if state == nil { + return + } + alt := alternateVariantKey(name) + if _, ok := state.Skills[alt]; !ok { + return + } + + altDir := filepath.Join(baseDir, alt) + removeSymlinksFromAgents(ctx, alt, altDir, scope, cwd) + if err := os.RemoveAll(altDir); err != nil { + log.Warnf(ctx, "Failed to remove previous variant %s: %v", altDir, err) + } + delete(state.Skills, alt) + cmdio.LogString(ctx, fmt.Sprintf("Replaced previous variant %s with %s", alt, name)) +} + // 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 func(ctx context.Context, ref, repoDir, skillName, filePath string) ([]byte, error) = fetchSkillFile @@ -73,10 +95,8 @@ func GetSkillsRef(ctx context.Context) (ref string, explicit bool, err error) { return "v" + v, false, nil } -// GetSkillsBaseURL returns the base URL for fetching the manifest and skill -// files. If DATABRICKS_SKILLS_BASE_URL is set, it returns that; otherwise the -// canonical raw.githubusercontent.com URL for the upstream repo. The override -// is used by acceptance tests to point fetches at a mock HTTP server. +// GetSkillsBaseURL returns the manifest and skill fetch base URL. +// DATABRICKS_SKILLS_BASE_URL overrides GitHub raw URLs for acceptance tests. func GetSkillsBaseURL(ctx context.Context) string { if base := env.Get(ctx, "DATABRICKS_SKILLS_BASE_URL"); base != "" { return strings.TrimRight(base, "/") @@ -85,11 +105,6 @@ func GetSkillsBaseURL(ctx context.Context) string { } // Manifest describes the skills manifest fetched from the skills repo. -// -// The repo exposes stable skills under skills/ and experimental skills under -// experimental/. Both appear in a single Skills map; each entry carries -// RepoDir indicating which top-level directory holds its files, which is -// also the source of truth for whether the skill is experimental. type Manifest struct { Version string `json:"version"` UpdatedAt string `json:"updated_at"` @@ -104,23 +119,15 @@ type SkillMeta struct { Description string `json:"description,omitempty"` MinCLIVer string `json:"min_cli_version,omitempty"` - // RepoDir is the top-level repo subdirectory (skills/ or experimental/) - // that holds this skill's files. Provided by the manifest; serves as - // the source of truth for whether the skill is experimental. + // RepoDir is "skills" or "experimental" (manifest field repo_dir). RepoDir string `json:"repo_dir,omitempty"` - // SourceName is the directory name within RepoDir that holds this - // skill's files in the upstream repo. For stable skills this equals - // the manifest key. For experimental skills the manifest key has a - // "-experimental" suffix appended for collision-free install paths, - // but the upstream repo dir does not — SourceName preserves it for - // the fetch URL. Populated during normalization; not part of the wire - // format. + // SourceName is the upstream skill directory name within RepoDir. + // For experimental skills the manifest key is suffixed (-experimental) + // but SourceName is not; set during normalization, not from JSON. SourceName string `json:"-"` } -// IsExperimental reports whether the skill is sourced from the experimental/ -// directory of the upstream repo. func (s SkillMeta) IsExperimental() bool { return s.RepoDir == experimentalRepoPath } @@ -192,8 +199,6 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent return err } - // Helpful nudge for users testing --experimental against a ref that - // pre-dates the experimental/ directory upstream. if opts.IncludeExperimental && !manifestHasExperimental(manifest) { log.Warnf(ctx, "--experimental was set but the manifest at %s exposes no experimental skills. Set DATABRICKS_SKILLS_REF to a release that includes them (or =main for the latest).", ref) } @@ -257,21 +262,7 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent for _, name := range skillNames { meta := targetSkills[name] - // Experimental↔stable transition: if the alternate variant of this - // skill was previously installed (upstream flipped its experimental - // status), remove the stale variant before installing the new one. - if state != nil { - alt := alternateVariantKey(name) - if _, ok := state.Skills[alt]; ok { - altDir := filepath.Join(baseDir, alt) - removeSymlinksFromAgents(ctx, alt, altDir, scope, cwd) - if err := os.RemoveAll(altDir); err != nil { - log.Warnf(ctx, "Failed to remove previous variant %s: %v", altDir, err) - } - delete(state.Skills, alt) - cmdio.LogString(ctx, fmt.Sprintf("Replaced previous variant %s with %s", alt, name)) - } - } + removeAlternateVariant(ctx, state, baseDir, name, scope, cwd) // Idempotency: skip if same version is already installed, the canonical // dir exists, AND every requested agent already has the skill on disk. diff --git a/libs/aitools/installer/installer_test.go b/libs/aitools/installer/installer_test.go index 36e818b9926..bc7aebf7f92 100644 --- a/libs/aitools/installer/installer_test.go +++ b/libs/aitools/installer/installer_test.go @@ -729,10 +729,6 @@ func TestInstallProjectScopeZeroCompatibleAgentsReturnsError(t *testing.T) { } func TestInstallReplacesAlternateVariant(t *testing.T) { - // Setup: a skill called "databricks-jobs" is installed as stable. - // Then the manifest re-categorizes it as experimental (key becomes - // "databricks-jobs-experimental"). A new install with --experimental - // should remove the stale stable variant and install the experimental one. tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) @@ -752,7 +748,6 @@ func TestInstallReplacesAlternateVariant(t *testing.T) { globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") require.DirExists(t, filepath.Join(globalDir, "databricks-jobs")) - // Now flip to experimental upstream. New install run. experimentalManifest := &Manifest{ Version: "1", Skills: map[string]SkillMeta{ @@ -766,9 +761,9 @@ func TestInstallReplacesAlternateVariant(t *testing.T) { state, err := LoadState(globalDir) require.NoError(t, err) - assert.NotContains(t, state.Skills, "databricks-jobs", "stale stable variant should be removed from state") + assert.NotContains(t, state.Skills, "databricks-jobs") assert.Equal(t, "0.2.0", state.Skills["databricks-jobs-experimental"]) - assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs"), "stale stable install dir should be gone") + assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs")) assert.DirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) assert.Contains(t, stderr.String(), "Replaced previous variant databricks-jobs with databricks-jobs-experimental") } diff --git a/libs/aitools/installer/source.go b/libs/aitools/installer/source.go index 6a7bf789a99..421e6f58489 100644 --- a/libs/aitools/installer/source.go +++ b/libs/aitools/installer/source.go @@ -53,14 +53,8 @@ func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (* return &manifest, nil } -// normalizeManifest stamps SourceName, defaults a missing RepoDir to the -// stable directory, and re-keys experimental skills with a "-experimental" -// suffix so their on-disk install paths can't collide with stable skills -// of the same name. SourceName preserves the upstream repo directory name -// (which does not carry the suffix) for fetch URLs. -// -// RepoDir is provided by the manifest and is the source of truth for -// whether a skill is experimental — see SkillMeta.IsExperimental. +// normalizeManifest stamps SourceName, defaults RepoDir, and suffixes +// experimental install keys while preserving unsuffixed upstream fetch paths. func normalizeManifest(m *Manifest) { if m.Skills == nil { m.Skills = map[string]SkillMeta{} diff --git a/libs/aitools/installer/source_test.go b/libs/aitools/installer/source_test.go index 12b6aec9c22..98c39d390c1 100644 --- a/libs/aitools/installer/source_test.go +++ b/libs/aitools/installer/source_test.go @@ -18,17 +18,17 @@ func TestNormalizeManifestStampsSourceNameAndSuffixesExperimental(t *testing.T) stable := m.Skills["databricks-apps"] assert.False(t, stable.IsExperimental()) - assert.Equal(t, stableSkillsRepoPath, stable.RepoDir, "missing RepoDir defaults to stable") - assert.Equal(t, "databricks-apps", stable.SourceName, "stable SourceName equals key") + assert.Equal(t, stableSkillsRepoPath, stable.RepoDir) + assert.Equal(t, "databricks-apps", stable.SourceName) exp, ok := m.Skills["databricks-iceberg-experimental"] - assert.True(t, ok, "experimental skills always get -experimental suffix on the manifest key") + assert.True(t, ok) assert.True(t, exp.IsExperimental()) assert.Equal(t, experimentalRepoPath, exp.RepoDir) assert.Equal(t, "databricks-iceberg", exp.SourceName, "SourceName preserves the upstream repo dir name (no suffix)") _, original := m.Skills["databricks-iceberg"] - assert.False(t, original, "the un-suffixed key should not be present") + assert.False(t, original) } func TestManifestHasExperimental(t *testing.T) { @@ -51,7 +51,6 @@ func TestManifestHasExperimental(t *testing.T) { func TestAlternateVariantKey(t *testing.T) { assert.Equal(t, "databricks-jobs-experimental", alternateVariantKey("databricks-jobs")) assert.Equal(t, "databricks-jobs", alternateVariantKey("databricks-jobs-experimental")) - // Idempotent under double application assert.Equal(t, "databricks-jobs", alternateVariantKey(alternateVariantKey("databricks-jobs"))) } diff --git a/libs/aitools/installer/uninstall.go b/libs/aitools/installer/uninstall.go index 821f49d6a29..acd289089bc 100644 --- a/libs/aitools/installer/uninstall.go +++ b/libs/aitools/installer/uninstall.go @@ -64,11 +64,9 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { if len(opts.Skills) > 0 { seen := make(map[string]bool) for _, name := range opts.Skills { - // Accept either variant: if the literal name isn't installed but - // the alternate variant is, use the alternate. This makes uninstall - // resilient to the experimental↔stable transition. if _, ok := state.Skills[name]; !ok { - if alt := alternateVariantKey(name); state.Skills[alt] != "" { + alt := alternateVariantKey(name) + if _, ok := state.Skills[alt]; ok { name = alt } else { return fmt.Errorf("skill %q is not installed", name) @@ -80,11 +78,11 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { seen[name] = true toRemove = append(toRemove, name) - // If both variants are installed, remove both (the alternate is - // the stale "old version" of the same logical skill). - if alt := alternateVariantKey(name); state.Skills[alt] != "" && !seen[alt] { - seen[alt] = true - toRemove = append(toRemove, alt) + if alt := alternateVariantKey(name); !seen[alt] { + if _, ok := state.Skills[alt]; ok { + seen[alt] = true + toRemove = append(toRemove, alt) + } } } } else { diff --git a/libs/aitools/installer/uninstall_test.go b/libs/aitools/installer/uninstall_test.go index 8f2c8906568..80cc543e11b 100644 --- a/libs/aitools/installer/uninstall_test.go +++ b/libs/aitools/installer/uninstall_test.go @@ -323,13 +323,6 @@ func TestUninstallSelectiveDuplicateNamesDeduplicates(t *testing.T) { } func TestUninstallByEitherVariantRemovesBoth(t *testing.T) { - // Setup: both stable and experimental variants of databricks-jobs are - // installed (the transitional state we want uninstall to clean up). - // The new manifest format only carries one entry per logical skill, so - // the both-installed state is constructed here by installing the stable - // variant via the manifest and seeding the experimental variant directly - // on disk + in state — simulating an install left over from a prior ref - // where the same skill lived under experimental/. tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) @@ -349,7 +342,7 @@ func TestUninstallByEitherVariantRemovesBoth(t *testing.T) { globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") require.DirExists(t, filepath.Join(globalDir, "databricks-jobs")) - // Seed the leftover experimental variant. + // Simulate a stale paired variant left over from an older manifest ref. expDir := filepath.Join(globalDir, "databricks-jobs-experimental") require.NoError(t, os.MkdirAll(expDir, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(expDir, "SKILL.md"), []byte("# exp"), 0o644)) @@ -358,7 +351,6 @@ func TestUninstallByEitherVariantRemovesBoth(t *testing.T) { state.Skills["databricks-jobs-experimental"] = "0.0.1" require.NoError(t, SaveState(globalDir, state)) - // Uninstall using just the un-suffixed name; both variants should go. require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{ Skills: []string{"databricks-jobs"}, })) @@ -369,7 +361,6 @@ func TestUninstallByEitherVariantRemovesBoth(t *testing.T) { } func TestUninstallByAlternateNameWhenOnlyOneVariantInstalled(t *testing.T) { - // Setup: only the experimental variant is installed. tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) @@ -389,8 +380,6 @@ func TestUninstallByAlternateNameWhenOnlyOneVariantInstalled(t *testing.T) { globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") require.DirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) - // User types the un-suffixed name (doesn't know which variant is on - // disk); uninstall should still find and remove it. require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{ Skills: []string{"databricks-jobs"}, })) diff --git a/libs/aitools/installer/update.go b/libs/aitools/installer/update.go index d1e8eecc4c2..3e386cda903 100644 --- a/libs/aitools/installer/update.go +++ b/libs/aitools/installer/update.go @@ -113,14 +113,15 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent for _, name := range names { meta, inManifest := manifest.Skills[name] - oldVersion := state.Skills[name] if !inManifest { - _, wasInstalled := state.Skills[name] - if wasInstalled { + if _, moved := manifest.Skills[alternateVariantKey(name)]; moved { + continue + } + if _, ok := state.Skills[name]; ok { log.Warnf(ctx, "Warning: %q not found in manifest %s (keeping installed version).", name, latestTag) + result.Unchanged = append(result.Unchanged, name) } - result.Unchanged = append(result.Unchanged, name) continue } @@ -138,10 +139,9 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent continue } - // Check if this is a new skill (not in state). - _, wasInstalled := state.Skills[name] + oldVersion, wasInstalled, alternateInstalled := installedSkillVersion(state, name) - if meta.Version == oldVersion && !opts.Force { + if meta.Version == oldVersion && !opts.Force && !alternateInstalled { result.Unchanged = append(result.Unchanged, name) continue } @@ -152,10 +152,10 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent NewVersion: meta.Version, } - if !wasInstalled { - result.Added = append(result.Added, update) - } else { + if wasInstalled || alternateInstalled { result.Updated = append(result.Updated, update) + } else { + result.Added = append(result.Added, update) } } @@ -177,6 +177,7 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent for _, change := range allChanges { meta := manifest.Skills[change.Name] + removeAlternateVariant(ctx, state, baseDir, change.Name, scope, cwd) if err := installSkillForAgents(ctx, change.Name, meta, targetAgents, params); err != nil { return nil, err } @@ -200,9 +201,9 @@ func buildUpdateSkillSet(state *InstallState, manifest *Manifest, opts UpdateOpt skillSet := make(map[string]bool) if len(opts.Skills) > 0 { - // Only named skills. for _, name := range opts.Skills { skillSet[name] = true + skillSet[alternateVariantKey(name)] = true } return skillSet } diff --git a/libs/aitools/installer/update_test.go b/libs/aitools/installer/update_test.go index e4b8d1d5953..8c4263fafa7 100644 --- a/libs/aitools/installer/update_test.go +++ b/libs/aitools/installer/update_test.go @@ -360,6 +360,107 @@ func TestUpdateSkipsExperimentalSkills(t *testing.T) { assert.Empty(t, result.Added) } +func TestUpdateReplacesAlternateVariant(t *testing.T) { + tests := []struct { + name string + installedManifest func() *Manifest + updatedManifest func() *Manifest + oldKey string + newKey string + }{ + { + name: "stable to experimental", + installedManifest: func() *Manifest { + return &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}}, + }, + } + }, + updatedManifest: func() *Manifest { + return &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.2.0", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, + }, + } + }, + oldKey: "databricks-jobs", + newKey: "databricks-jobs-experimental", + }, + { + name: "experimental to stable", + installedManifest: func() *Manifest { + return &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, + }, + } + }, + updatedManifest: func() *Manifest { + return &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.2.0", Files: []string{"SKILL.md"}}, + }, + } + }, + oldKey: "databricks-jobs-experimental", + newKey: "databricks-jobs", + }, + } + + for _, tt := range tests { + for _, targeted := range []bool{false, true} { + mode := "all" + opts := UpdateOptions{} + if targeted { + mode = "targeted" + opts.Skills = []string{tt.oldKey} + } + + t.Run(tt.name+" "+mode, func(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) + agent := testAgent(tmp) + + require.NoError(t, InstallSkillsForAgents( + ctx, &mockManifestSource{manifest: tt.installedManifest()}, + []*agents.Agent{agent}, InstallOptions{IncludeExperimental: true}, + )) + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + require.DirExists(t, filepath.Join(globalDir, tt.oldKey)) + + t.Setenv("DATABRICKS_SKILLS_REF", "v0.2.0") + result, err := UpdateSkills( + ctx, &mockManifestSource{manifest: tt.updatedManifest()}, + []*agents.Agent{agent}, opts, + ) + require.NoError(t, err) + + require.Len(t, result.Updated, 1) + assert.Equal(t, tt.newKey, result.Updated[0].Name) + assert.Equal(t, "0.1.0", result.Updated[0].OldVersion) + assert.Equal(t, "0.2.0", result.Updated[0].NewVersion) + assert.NotContains(t, result.Unchanged, tt.oldKey) + assert.Empty(t, result.Added) + + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.NotContains(t, state.Skills, tt.oldKey) + assert.Equal(t, "0.2.0", state.Skills[tt.newKey]) + assert.NoDirExists(t, filepath.Join(globalDir, tt.oldKey)) + assert.DirExists(t, filepath.Join(globalDir, tt.newKey)) + }) + } + } +} + func TestUpdateSkipsMinCLIVersionSkills(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) From 6d9c479fbfc7ee760b152ff651888b334857b647 Mon Sep 17 00:00:00 2001 From: simon Date: Wed, 20 May 2026 17:08:50 +0200 Subject: [PATCH 4/5] aitools: keep experimental skill names stable Use repo_dir as the experimental signal without suffixing install keys, and track repo dirs in state so source-folder moves still refresh installed files. --- .../skills/install-specific/output.txt | 8 +- .../aitools/skills/install-specific/script | 6 +- libs/aitools/installer/installer.go | 53 +++-------- libs/aitools/installer/installer_test.go | 29 +++--- libs/aitools/installer/source.go | 11 +-- libs/aitools/installer/source_test.go | 18 +--- libs/aitools/installer/state.go | 1 + libs/aitools/installer/state_test.go | 7 ++ libs/aitools/installer/uninstall.go | 15 +--- libs/aitools/installer/uninstall_test.go | 64 ------------- libs/aitools/installer/update.go | 16 ++-- libs/aitools/installer/update_test.go | 89 +++++++++++++++---- 12 files changed, 132 insertions(+), 185 deletions(-) diff --git a/acceptance/experimental/aitools/skills/install-specific/output.txt b/acceptance/experimental/aitools/skills/install-specific/output.txt index e38470435b2..dc5ebd3e572 100644 --- a/acceptance/experimental/aitools/skills/install-specific/output.txt +++ b/acceptance/experimental/aitools/skills/install-specific/output.txt @@ -6,18 +6,18 @@ Installing Databricks AI skills for Claude Code... Using skills version test-ref Installed 1 skill. -=== install a specific experimental skill (note the -experimental suffix) ->>> [CLI] experimental aitools install --global --skills test-exp-experimental --experimental +=== install a specific experimental skill +>>> [CLI] experimental aitools install --global --skills test-exp --experimental Command "install" is deprecated, use "databricks aitools install" instead. Installing Databricks AI skills for Claude Code... Using skills version test-ref Installed 1 skill. === asking for an experimental skill without --experimental flag errors out ->>> [CLI] experimental aitools install --global --skills test-exp-experimental +>>> [CLI] experimental aitools install --global --skills test-exp Command "install" is deprecated, use "databricks aitools install" instead. Installing Databricks AI skills for Claude Code... Using skills version test-ref -Error: skill "test-exp-experimental" is experimental; use --experimental to install +Error: skill "test-exp" is experimental; use --experimental to install Exit code: 1 diff --git a/acceptance/experimental/aitools/skills/install-specific/script b/acceptance/experimental/aitools/skills/install-specific/script index e6eac21d461..f87aa4fbd24 100644 --- a/acceptance/experimental/aitools/skills/install-specific/script +++ b/acceptance/experimental/aitools/skills/install-specific/script @@ -4,8 +4,8 @@ mkdir -p "${USERPROFILE:-$HOME}/.claude" title "install only one specific stable skill via --skills" trace $CLI experimental aitools install --global --skills test-stable-a -title "install a specific experimental skill (note the -experimental suffix)" -trace $CLI experimental aitools install --global --skills test-exp-experimental --experimental +title "install a specific experimental skill" +trace $CLI experimental aitools install --global --skills test-exp --experimental title "asking for an experimental skill without --experimental flag errors out" -errcode trace $CLI experimental aitools install --global --skills test-exp-experimental +errcode trace $CLI experimental aitools install --global --skills test-exp diff --git a/libs/aitools/installer/installer.go b/libs/aitools/installer/installer.go index b454e5a8aea..548be8dcf3e 100644 --- a/libs/aitools/installer/installer.go +++ b/libs/aitools/installer/installer.go @@ -28,7 +28,6 @@ const ( skillsRepoName = "databricks-agent-skills" stableSkillsRepoPath = "skills" experimentalRepoPath = "experimental" - experimentalSuffix = "-experimental" ) func manifestHasExperimental(m *Manifest) bool { @@ -40,41 +39,13 @@ func manifestHasExperimental(m *Manifest) bool { return false } -// alternateVariantKey maps between "foo" and "foo-experimental". -func alternateVariantKey(key string) string { - if base, ok := strings.CutSuffix(key, experimentalSuffix); ok { - return base - } - return key + experimentalSuffix -} - -// installedSkillVersion returns the recorded version for name, or for its -// stable/experimental alternate when only that variant is installed. -func installedSkillVersion(state *InstallState, name string) (version string, selfInstalled, alternateInstalled bool) { - version, selfInstalled = state.Skills[name] - altVersion, alternateInstalled := state.Skills[alternateVariantKey(name)] - if !selfInstalled && alternateInstalled { - version = altVersion - } - return version, selfInstalled, alternateInstalled -} - -func removeAlternateVariant(ctx context.Context, state *InstallState, baseDir, name, scope, cwd string) { - if state == nil { - return - } - alt := alternateVariantKey(name) - if _, ok := state.Skills[alt]; !ok { - return - } - - altDir := filepath.Join(baseDir, alt) - removeSymlinksFromAgents(ctx, alt, altDir, scope, cwd) - if err := os.RemoveAll(altDir); err != nil { - log.Warnf(ctx, "Failed to remove previous variant %s: %v", altDir, err) +func stateRepoDir(state *InstallState, name string) string { + if state != nil && state.RepoDirs != nil { + if repoDir := state.RepoDirs[name]; repoDir != "" { + return repoDir + } } - delete(state.Skills, alt) - cmdio.LogString(ctx, fmt.Sprintf("Replaced previous variant %s with %s", alt, name)) + return stableSkillsRepoPath } // fetchFileFn is the function used to download individual skill files. @@ -123,8 +94,7 @@ type SkillMeta struct { RepoDir string `json:"repo_dir,omitempty"` // SourceName is the upstream skill directory name within RepoDir. - // For experimental skills the manifest key is suffixed (-experimental) - // but SourceName is not; set during normalization, not from JSON. + // Set during normalization, not from JSON. SourceName string `json:"-"` } @@ -262,11 +232,9 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent for _, name := range skillNames { meta := targetSkills[name] - removeAlternateVariant(ctx, state, baseDir, name, scope, cwd) - // Idempotency: skip if same version is already installed, the canonical // dir exists, AND every requested agent already has the skill on disk. - if state != nil && state.Skills[name] == meta.Version { + if state != nil && state.Skills[name] == meta.Version && stateRepoDir(state, name) == meta.RepoDir { skillDir := filepath.Join(baseDir, name) if _, statErr := os.Stat(skillDir); statErr == nil && allAgentsHaveSkill(ctx, name, targetAgents, scope, cwd) { log.Debugf(ctx, "%s v%s already installed for all agents, skipping", name, meta.Version) @@ -285,8 +253,12 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent state = &InstallState{ SchemaVersion: 1, Skills: make(map[string]string, len(targetSkills)), + RepoDirs: make(map[string]string, len(targetSkills)), } } + if state.RepoDirs == nil { + state.RepoDirs = make(map[string]string, len(state.Skills)+len(targetSkills)) + } state.Release = ref state.LastUpdated = time.Now() // IncludeExperimental reflects the last invocation's flag value. The Skills @@ -296,6 +268,7 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent state.Scope = scope for name, meta := range targetSkills { state.Skills[name] = meta.Version + state.RepoDirs[name] = meta.RepoDir } if err := SaveState(baseDir, state); err != nil { return err diff --git a/libs/aitools/installer/installer_test.go b/libs/aitools/installer/installer_test.go index bc7aebf7f92..710f4861439 100644 --- a/libs/aitools/installer/installer_test.go +++ b/libs/aitools/installer/installer_test.go @@ -279,7 +279,7 @@ func TestExperimentalSkillsSkippedByDefault(t *testing.T) { require.NoError(t, err) // Only non-experimental skills should be installed. assert.Len(t, state.Skills, 2) - assert.NotContains(t, state.Skills, "databricks-iceberg-experimental") + assert.NotContains(t, state.Skills, "databricks-iceberg") assert.Contains(t, stderr.String(), "Installed 2 skills.") } @@ -309,7 +309,7 @@ func TestExperimentalSkillsIncludedWithFlag(t *testing.T) { state, err := LoadState(globalDir) require.NoError(t, err) assert.Len(t, state.Skills, 3) - assert.Contains(t, state.Skills, "databricks-iceberg-experimental") + assert.Contains(t, state.Skills, "databricks-iceberg") assert.True(t, state.IncludeExperimental) assert.Contains(t, stderr.String(), "Installed 3 skills.") @@ -728,11 +728,17 @@ func TestInstallProjectScopeZeroCompatibleAgentsReturnsError(t *testing.T) { assert.Contains(t, err.Error(), "No Project Agent") } -func TestInstallReplacesAlternateVariant(t *testing.T) { +func TestInstallKeepsNameWhenRepoDirChanges(t *testing.T) { tmp := setupTestHome(t) - ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) - setupFetchMock(t) + ctx := cmdio.MockDiscard(t.Context()) agent := testAgent(tmp) + var fetchedFrom []string + orig := fetchFileFn + t.Cleanup(func() { fetchFileFn = orig }) + fetchFileFn = func(_ context.Context, _, repoDir, skillName, filePath string) ([]byte, error) { + fetchedFrom = append(fetchedFrom, filepath.Join(repoDir, skillName, filePath)) + return []byte("# " + skillName + "/" + filePath), nil + } stableManifest := &Manifest{ Version: "1", @@ -747,11 +753,13 @@ func TestInstallReplacesAlternateVariant(t *testing.T) { globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") require.DirExists(t, filepath.Join(globalDir, "databricks-jobs")) + assert.Contains(t, fetchedFrom, filepath.Join(stableSkillsRepoPath, "databricks-jobs", "SKILL.md")) + fetchedFrom = nil experimentalManifest := &Manifest{ Version: "1", Skills: map[string]SkillMeta{ - "databricks-jobs": {Version: "0.2.0", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, }, } require.NoError(t, InstallSkillsForAgents( @@ -761,11 +769,10 @@ func TestInstallReplacesAlternateVariant(t *testing.T) { state, err := LoadState(globalDir) require.NoError(t, err) - assert.NotContains(t, state.Skills, "databricks-jobs") - assert.Equal(t, "0.2.0", state.Skills["databricks-jobs-experimental"]) - assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs")) - assert.DirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) - assert.Contains(t, stderr.String(), "Replaced previous variant databricks-jobs with databricks-jobs-experimental") + assert.Equal(t, "0.1.0", state.Skills["databricks-jobs"]) + assert.Equal(t, experimentalRepoPath, state.RepoDirs["databricks-jobs"]) + assert.DirExists(t, filepath.Join(globalDir, "databricks-jobs")) + assert.Contains(t, fetchedFrom, filepath.Join(experimentalRepoPath, "databricks-jobs", "SKILL.md")) } func TestSupportsProjectScopeSetCorrectly(t *testing.T) { diff --git a/libs/aitools/installer/source.go b/libs/aitools/installer/source.go index 421e6f58489..f3d9842f876 100644 --- a/libs/aitools/installer/source.go +++ b/libs/aitools/installer/source.go @@ -53,23 +53,16 @@ func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (* return &manifest, nil } -// normalizeManifest stamps SourceName, defaults RepoDir, and suffixes -// experimental install keys while preserving unsuffixed upstream fetch paths. +// normalizeManifest stamps SourceName and defaults RepoDir for older manifests. func normalizeManifest(m *Manifest) { if m.Skills == nil { m.Skills = map[string]SkillMeta{} } - out := make(map[string]SkillMeta, len(m.Skills)) for name, meta := range m.Skills { if meta.RepoDir == "" { meta.RepoDir = stableSkillsRepoPath } meta.SourceName = name - if meta.IsExperimental() { - out[name+experimentalSuffix] = meta - } else { - out[name] = meta - } + m.Skills[name] = meta } - m.Skills = out } diff --git a/libs/aitools/installer/source_test.go b/libs/aitools/installer/source_test.go index 98c39d390c1..fb52563b686 100644 --- a/libs/aitools/installer/source_test.go +++ b/libs/aitools/installer/source_test.go @@ -6,7 +6,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestNormalizeManifestStampsSourceNameAndSuffixesExperimental(t *testing.T) { +func TestNormalizeManifestStampsSourceNameAndRepoDir(t *testing.T) { m := &Manifest{ Skills: map[string]SkillMeta{ "databricks-apps": {Version: "0.1.0", Files: []string{"SKILL.md"}}, @@ -21,14 +21,11 @@ func TestNormalizeManifestStampsSourceNameAndSuffixesExperimental(t *testing.T) assert.Equal(t, stableSkillsRepoPath, stable.RepoDir) assert.Equal(t, "databricks-apps", stable.SourceName) - exp, ok := m.Skills["databricks-iceberg-experimental"] + exp, ok := m.Skills["databricks-iceberg"] assert.True(t, ok) assert.True(t, exp.IsExperimental()) assert.Equal(t, experimentalRepoPath, exp.RepoDir) - assert.Equal(t, "databricks-iceberg", exp.SourceName, "SourceName preserves the upstream repo dir name (no suffix)") - - _, original := m.Skills["databricks-iceberg"] - assert.False(t, original) + assert.Equal(t, "databricks-iceberg", exp.SourceName) } func TestManifestHasExperimental(t *testing.T) { @@ -48,13 +45,6 @@ func TestManifestHasExperimental(t *testing.T) { assert.True(t, manifestHasExperimental(withExperimental)) } -func TestAlternateVariantKey(t *testing.T) { - assert.Equal(t, "databricks-jobs-experimental", alternateVariantKey("databricks-jobs")) - assert.Equal(t, "databricks-jobs", alternateVariantKey("databricks-jobs-experimental")) - assert.Equal(t, "databricks-jobs", - alternateVariantKey(alternateVariantKey("databricks-jobs"))) -} - func TestNormalizeManifestOnlyExperimentalSkills(t *testing.T) { m := &Manifest{ Skills: map[string]SkillMeta{ @@ -64,7 +54,7 @@ func TestNormalizeManifestOnlyExperimentalSkills(t *testing.T) { normalizeManifest(m) - got, ok := m.Skills["x-experimental"] + got, ok := m.Skills["x"] assert.True(t, ok) assert.True(t, got.IsExperimental()) assert.Equal(t, experimentalRepoPath, got.RepoDir) diff --git a/libs/aitools/installer/state.go b/libs/aitools/installer/state.go index a666a585057..9aa52359bd5 100644 --- a/libs/aitools/installer/state.go +++ b/libs/aitools/installer/state.go @@ -27,6 +27,7 @@ type InstallState struct { Release string `json:"release"` LastUpdated time.Time `json:"last_updated"` Skills map[string]string `json:"skills"` + RepoDirs map[string]string `json:"repo_dirs,omitempty"` Scope string `json:"scope,omitempty"` } diff --git a/libs/aitools/installer/state_test.go b/libs/aitools/installer/state_test.go index f1fcdb8c229..4ed1e78d5f2 100644 --- a/libs/aitools/installer/state_test.go +++ b/libs/aitools/installer/state_test.go @@ -26,6 +26,9 @@ func TestSaveAndLoadStateRoundtrip(t *testing.T) { Skills: map[string]string{ "databricks": "1.0.0", }, + RepoDirs: map[string]string{ + "databricks": stableSkillsRepoPath, + }, } err := SaveState(dir, original) @@ -105,6 +108,10 @@ func TestSaveAndLoadStateWithOptionalFields(t *testing.T) { "databricks": "1.0.0", "sql-tools": "0.2.0", }, + RepoDirs: map[string]string{ + "databricks": stableSkillsRepoPath, + "sql-tools": experimentalRepoPath, + }, Scope: "project", } diff --git a/libs/aitools/installer/uninstall.go b/libs/aitools/installer/uninstall.go index acd289089bc..2e97fa784a5 100644 --- a/libs/aitools/installer/uninstall.go +++ b/libs/aitools/installer/uninstall.go @@ -65,25 +65,13 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { seen := make(map[string]bool) for _, name := range opts.Skills { if _, ok := state.Skills[name]; !ok { - alt := alternateVariantKey(name) - if _, ok := state.Skills[alt]; ok { - name = alt - } else { - return fmt.Errorf("skill %q is not installed", name) - } + return fmt.Errorf("skill %q is not installed", name) } if seen[name] { continue } seen[name] = true toRemove = append(toRemove, name) - - if alt := alternateVariantKey(name); !seen[alt] { - if _, ok := state.Skills[alt]; ok { - seen[alt] = true - toRemove = append(toRemove, alt) - } - } } } else { for name := range state.Skills { @@ -101,6 +89,7 @@ func UninstallSkillsOpts(ctx context.Context, opts UninstallOptions) error { log.Warnf(ctx, "Failed to remove %s: %v", canonicalDir, err) } delete(state.Skills, name) + delete(state.RepoDirs, name) } if removeAll { diff --git a/libs/aitools/installer/uninstall_test.go b/libs/aitools/installer/uninstall_test.go index 80cc543e11b..3dc8e03af35 100644 --- a/libs/aitools/installer/uninstall_test.go +++ b/libs/aitools/installer/uninstall_test.go @@ -322,70 +322,6 @@ func TestUninstallSelectiveDuplicateNamesDeduplicates(t *testing.T) { assert.Contains(t, stderr.String(), "Uninstalled 1 skill.") } -func TestUninstallByEitherVariantRemovesBoth(t *testing.T) { - tmp := setupTestHome(t) - ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) - setupFetchMock(t) - agent := testAgent(tmp) - - stableManifest := &Manifest{ - Version: "1", - Skills: map[string]SkillMeta{ - "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}}, - }, - } - require.NoError(t, InstallSkillsForAgents( - ctx, &mockManifestSource{manifest: stableManifest}, - []*agents.Agent{agent}, InstallOptions{}, - )) - - globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") - require.DirExists(t, filepath.Join(globalDir, "databricks-jobs")) - - // Simulate a stale paired variant left over from an older manifest ref. - expDir := filepath.Join(globalDir, "databricks-jobs-experimental") - require.NoError(t, os.MkdirAll(expDir, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(expDir, "SKILL.md"), []byte("# exp"), 0o644)) - state, err := LoadState(globalDir) - require.NoError(t, err) - state.Skills["databricks-jobs-experimental"] = "0.0.1" - require.NoError(t, SaveState(globalDir, state)) - - require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{ - Skills: []string{"databricks-jobs"}, - })) - - assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs")) - assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) - assert.Contains(t, stderr.String(), "Uninstalled 2 skills.") -} - -func TestUninstallByAlternateNameWhenOnlyOneVariantInstalled(t *testing.T) { - tmp := setupTestHome(t) - ctx := cmdio.MockDiscard(t.Context()) - setupFetchMock(t) - agent := testAgent(tmp) - - manifest := &Manifest{ - Version: "1", - Skills: map[string]SkillMeta{ - "databricks-jobs": {Version: "0.0.1", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, - }, - } - require.NoError(t, InstallSkillsForAgents( - ctx, &mockManifestSource{manifest: manifest}, - []*agents.Agent{agent}, InstallOptions{IncludeExperimental: true}, - )) - - globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") - require.DirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) - - require.NoError(t, UninstallSkillsOpts(ctx, UninstallOptions{ - Skills: []string{"databricks-jobs"}, - })) - assert.NoDirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) -} - func TestUninstallSelectiveAllRemovesStateFile(t *testing.T) { tmp := setupTestHome(t) globalDir := installTestSkills(t, tmp) diff --git a/libs/aitools/installer/update.go b/libs/aitools/installer/update.go index 3e386cda903..39db94c8f62 100644 --- a/libs/aitools/installer/update.go +++ b/libs/aitools/installer/update.go @@ -115,9 +115,6 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent meta, inManifest := manifest.Skills[name] if !inManifest { - if _, moved := manifest.Skills[alternateVariantKey(name)]; moved { - continue - } if _, ok := state.Skills[name]; ok { log.Warnf(ctx, "Warning: %q not found in manifest %s (keeping installed version).", name, latestTag) result.Unchanged = append(result.Unchanged, name) @@ -139,9 +136,9 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent continue } - oldVersion, wasInstalled, alternateInstalled := installedSkillVersion(state, name) + oldVersion, wasInstalled := state.Skills[name] - if meta.Version == oldVersion && !opts.Force && !alternateInstalled { + if meta.Version == oldVersion && stateRepoDir(state, name) == meta.RepoDir && !opts.Force { result.Unchanged = append(result.Unchanged, name) continue } @@ -152,7 +149,7 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent NewVersion: meta.Version, } - if wasInstalled || alternateInstalled { + if wasInstalled { result.Updated = append(result.Updated, update) } else { result.Added = append(result.Added, update) @@ -177,7 +174,6 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent for _, change := range allChanges { meta := manifest.Skills[change.Name] - removeAlternateVariant(ctx, state, baseDir, change.Name, scope, cwd) if err := installSkillForAgents(ctx, change.Name, meta, targetAgents, params); err != nil { return nil, err } @@ -186,8 +182,13 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent // Update state. state.Release = latestTag state.LastUpdated = time.Now() + if state.RepoDirs == nil { + state.RepoDirs = make(map[string]string, len(state.Skills)+len(allChanges)) + } for _, change := range allChanges { + meta := manifest.Skills[change.Name] state.Skills[change.Name] = change.NewVersion + state.RepoDirs[change.Name] = meta.RepoDir } if err := SaveState(baseDir, state); err != nil { return nil, err @@ -203,7 +204,6 @@ func buildUpdateSkillSet(state *InstallState, manifest *Manifest, opts UpdateOpt if len(opts.Skills) > 0 { for _, name := range opts.Skills { skillSet[name] = true - skillSet[alternateVariantKey(name)] = true } return skillSet } diff --git a/libs/aitools/installer/update_test.go b/libs/aitools/installer/update_test.go index 8c4263fafa7..72f5a6b25f2 100644 --- a/libs/aitools/installer/update_test.go +++ b/libs/aitools/installer/update_test.go @@ -356,17 +356,16 @@ func TestUpdateSkipsExperimentalSkills(t *testing.T) { require.NoError(t, err) // Experimental skill should be skipped. - assert.Contains(t, result.Skipped, "databricks-iceberg-experimental") + assert.Contains(t, result.Skipped, "databricks-iceberg") assert.Empty(t, result.Added) } -func TestUpdateReplacesAlternateVariant(t *testing.T) { +func TestUpdateKeepsNameWhenRepoDirChanges(t *testing.T) { tests := []struct { name string installedManifest func() *Manifest updatedManifest func() *Manifest - oldKey string - newKey string + wantRepoDir string }{ { name: "stable to experimental", @@ -382,12 +381,11 @@ func TestUpdateReplacesAlternateVariant(t *testing.T) { return &Manifest{ Version: "1", Skills: map[string]SkillMeta{ - "databricks-jobs": {Version: "0.2.0", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, }, } }, - oldKey: "databricks-jobs", - newKey: "databricks-jobs-experimental", + wantRepoDir: experimentalRepoPath, }, { name: "experimental to stable", @@ -403,12 +401,11 @@ func TestUpdateReplacesAlternateVariant(t *testing.T) { return &Manifest{ Version: "1", Skills: map[string]SkillMeta{ - "databricks-jobs": {Version: "0.2.0", Files: []string{"SKILL.md"}}, + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}}, }, } }, - oldKey: "databricks-jobs-experimental", - newKey: "databricks-jobs", + wantRepoDir: stableSkillsRepoPath, }, } @@ -418,7 +415,7 @@ func TestUpdateReplacesAlternateVariant(t *testing.T) { opts := UpdateOptions{} if targeted { mode = "targeted" - opts.Skills = []string{tt.oldKey} + opts.Skills = []string{"databricks-jobs"} } t.Run(tt.name+" "+mode, func(t *testing.T) { @@ -434,7 +431,7 @@ func TestUpdateReplacesAlternateVariant(t *testing.T) { )) globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") - require.DirExists(t, filepath.Join(globalDir, tt.oldKey)) + require.DirExists(t, filepath.Join(globalDir, "databricks-jobs")) t.Setenv("DATABRICKS_SKILLS_REF", "v0.2.0") result, err := UpdateSkills( @@ -444,23 +441,77 @@ func TestUpdateReplacesAlternateVariant(t *testing.T) { require.NoError(t, err) require.Len(t, result.Updated, 1) - assert.Equal(t, tt.newKey, result.Updated[0].Name) + assert.Equal(t, "databricks-jobs", result.Updated[0].Name) assert.Equal(t, "0.1.0", result.Updated[0].OldVersion) - assert.Equal(t, "0.2.0", result.Updated[0].NewVersion) - assert.NotContains(t, result.Unchanged, tt.oldKey) + assert.Equal(t, "0.1.0", result.Updated[0].NewVersion) + assert.NotContains(t, result.Unchanged, "databricks-jobs") assert.Empty(t, result.Added) state, err := LoadState(globalDir) require.NoError(t, err) - assert.NotContains(t, state.Skills, tt.oldKey) - assert.Equal(t, "0.2.0", state.Skills[tt.newKey]) - assert.NoDirExists(t, filepath.Join(globalDir, tt.oldKey)) - assert.DirExists(t, filepath.Join(globalDir, tt.newKey)) + assert.Equal(t, "0.1.0", state.Skills["databricks-jobs"]) + assert.Equal(t, tt.wantRepoDir, state.RepoDirs["databricks-jobs"]) + assert.DirExists(t, filepath.Join(globalDir, "databricks-jobs")) }) } } } +func TestUpdateRepoDirChangeFromLegacyState(t *testing.T) { + tmp := setupTestHome(t) + ctx := cmdio.MockDiscard(t.Context()) + agent := testAgent(tmp) + t.Setenv("DATABRICKS_SKILLS_REF", "v0.2.0") + + globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") + require.NoError(t, SaveState(globalDir, &InstallState{ + SchemaVersion: 1, + IncludeExperimental: true, + Release: testSkillsRef, + Skills: map[string]string{ + "databricks-jobs": "0.1.0", + }, + })) + + fetchCalls := 0 + orig := fetchFileFn + t.Cleanup(func() { fetchFileFn = orig }) + fetchFileFn = func(_ context.Context, _, _, _, _ string) ([]byte, error) { + fetchCalls++ + return []byte("content"), nil + } + + stableManifest := &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}}, + }, + } + stableResult, err := UpdateSkills(ctx, &mockManifestSource{manifest: stableManifest}, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + assert.Contains(t, stableResult.Unchanged, "databricks-jobs") + assert.Equal(t, 0, fetchCalls) + + t.Setenv("DATABRICKS_SKILLS_REF", "v0.3.0") + experimentalManifest := &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}, RepoDir: experimentalRepoPath}, + }, + } + experimentalResult, err := UpdateSkills(ctx, &mockManifestSource{manifest: experimentalManifest}, []*agents.Agent{agent}, UpdateOptions{}) + require.NoError(t, err) + require.Len(t, experimentalResult.Updated, 1) + assert.Equal(t, "databricks-jobs", experimentalResult.Updated[0].Name) + assert.Equal(t, "0.1.0", experimentalResult.Updated[0].OldVersion) + assert.Equal(t, "0.1.0", experimentalResult.Updated[0].NewVersion) + assert.Positive(t, fetchCalls) + + state, err := LoadState(globalDir) + require.NoError(t, err) + assert.Equal(t, experimentalRepoPath, state.RepoDirs["databricks-jobs"]) +} + func TestUpdateSkipsMinCLIVersionSkills(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) From 19724be08c3680134d7bd19e1699da8de6e340c0 Mon Sep 17 00:00:00 2001 From: James Broadhead Date: Wed, 20 May 2026 16:44:34 +0000 Subject: [PATCH 5/5] aitools: refresh skills install goldens for --global deprecation warning #5234 added a deprecation message when --global is passed; regenerate the affected aitools install goldens so the acceptance tests pass after the merge from main. Co-authored-by: Isaac --- .../aitools/skills/install-experimental-empty/output.txt | 1 + .../experimental/aitools/skills/install-specific/output.txt | 3 +++ acceptance/experimental/aitools/skills/install/output.txt | 3 +++ 3 files changed, 7 insertions(+) diff --git a/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt b/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt index 776e8c85ffb..78a0d503215 100644 --- a/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt @@ -2,6 +2,7 @@ === --experimental against a manifest with no experimental skills logs a nudge >>> [CLI] experimental aitools install --global --experimental Command "install" is deprecated, use "databricks aitools install" instead. +Flag --global has been deprecated, use --scope=global Installing Databricks AI skills for Claude Code... Using skills version test-ref Warn: --experimental was set but the manifest at test-ref exposes no experimental skills. Set DATABRICKS_SKILLS_REF to a release that includes them (or =main for the latest). diff --git a/acceptance/experimental/aitools/skills/install-specific/output.txt b/acceptance/experimental/aitools/skills/install-specific/output.txt index dc5ebd3e572..f6222311b1b 100644 --- a/acceptance/experimental/aitools/skills/install-specific/output.txt +++ b/acceptance/experimental/aitools/skills/install-specific/output.txt @@ -2,6 +2,7 @@ === install only one specific stable skill via --skills >>> [CLI] experimental aitools install --global --skills test-stable-a Command "install" is deprecated, use "databricks aitools install" instead. +Flag --global has been deprecated, use --scope=global Installing Databricks AI skills for Claude Code... Using skills version test-ref Installed 1 skill. @@ -9,6 +10,7 @@ Installed 1 skill. === install a specific experimental skill >>> [CLI] experimental aitools install --global --skills test-exp --experimental Command "install" is deprecated, use "databricks aitools install" instead. +Flag --global has been deprecated, use --scope=global Installing Databricks AI skills for Claude Code... Using skills version test-ref Installed 1 skill. @@ -16,6 +18,7 @@ Installed 1 skill. === asking for an experimental skill without --experimental flag errors out >>> [CLI] experimental aitools install --global --skills test-exp Command "install" is deprecated, use "databricks aitools install" instead. +Flag --global has been deprecated, use --scope=global Installing Databricks AI skills for Claude Code... Using skills version test-ref Error: skill "test-exp" is experimental; use --experimental to install diff --git a/acceptance/experimental/aitools/skills/install/output.txt b/acceptance/experimental/aitools/skills/install/output.txt index b33021d09b3..f659c9e2998 100644 --- a/acceptance/experimental/aitools/skills/install/output.txt +++ b/acceptance/experimental/aitools/skills/install/output.txt @@ -2,6 +2,7 @@ === stable-only install (no --experimental flag) installs 1 skill >>> [CLI] experimental aitools install --global Command "install" is deprecated, use "databricks aitools install" instead. +Flag --global has been deprecated, use --scope=global Installing Databricks AI skills for Claude Code... Using skills version test-ref Installed 1 skill. @@ -9,6 +10,7 @@ Installed 1 skill. === re-run with --experimental installs the experimental one too >>> [CLI] experimental aitools install --global --experimental Command "install" is deprecated, use "databricks aitools install" instead. +Flag --global has been deprecated, use --scope=global Installing Databricks AI skills for Claude Code... Using skills version test-ref Installed 2 skills. @@ -16,6 +18,7 @@ Installed 2 skills. === no-op re-run is idempotent (no new fetches, no errors) >>> [CLI] experimental aitools install --global --experimental Command "install" is deprecated, use "databricks aitools install" instead. +Flag --global has been deprecated, use --scope=global Installing Databricks AI skills for Claude Code... Using skills version test-ref Installed 2 skills.