diff --git a/Taskfile.yml b/Taskfile.yml index 38f690bd56..c50a945cbd 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -613,6 +613,7 @@ tasks: sources: - experimental/aitools/** - acceptance/apps/** + - acceptance/experimental/aitools/** - "{{.EMBED_SOURCES}}" cmds: - | @@ -626,7 +627,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 0000000000..d6187dcb04 --- /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 0000000000..ecc6466b04 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-experimental-empty/output.txt @@ -0,0 +1,6 @@ + +=== --experimental against a manifest with no experimental skills logs a nudge +>>> [CLI] experimental aitools install --global --experimental +Installing Databricks AI skills for Claude Code... +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 (vtest-ref). 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 0000000000..56fca80e38 --- /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 0000000000..62a3ebea74 --- /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 experimental_skills section. +# 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 0000000000..d6187dcb04 --- /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 0000000000..3cb076f3f0 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-specific/output.txt @@ -0,0 +1,17 @@ + +=== install only one specific stable skill via --skills +>>> [CLI] experimental aitools install --global --skills test-stable-a +Installing Databricks AI skills for Claude Code... +Installed 1 skill (vtest-ref). + +=== install a specific experimental skill (note the -experimental suffix) +>>> [CLI] experimental aitools install --global --skills test-exp-experimental --experimental +Installing Databricks AI skills for Claude Code... +Installed 1 skill (vtest-ref). + +=== asking for an experimental skill without --experimental flag errors out +>>> [CLI] experimental aitools install --global --skills test-exp-experimental +Installing Databricks AI skills for Claude Code... +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 0000000000..f54391dd19 --- /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 0000000000..39889ab064 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install-specific/test.toml @@ -0,0 +1,52 @@ +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"]}, + "test-stable-b": {"version": "1.0.0", "files": ["SKILL.md"]} + }, + "experimental_skills": { + "test-exp": {"version": "0.0.1", "files": ["SKILL.md"]} + } +} +''' + +[[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 0000000000..d6187dcb04 --- /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 0000000000..841927b572 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install/output.txt @@ -0,0 +1,15 @@ + +=== stable-only install (no --experimental flag) installs 1 skill +>>> [CLI] experimental aitools install --global +Installing Databricks AI skills for Claude Code... +Installed 1 skill (vtest-ref). + +=== re-run with --experimental installs the experimental one too +>>> [CLI] experimental aitools install --global --experimental +Installing Databricks AI skills for Claude Code... +Installed 2 skills (vtest-ref). + +=== no-op re-run is idempotent (no new fetches, no errors) +>>> [CLI] experimental aitools install --global --experimental +Installing Databricks AI skills for Claude Code... +Installed 2 skills (vtest-ref). diff --git a/acceptance/experimental/aitools/skills/install/script b/acceptance/experimental/aitools/skills/install/script new file mode 100644 index 0000000000..616e4b22c6 --- /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 0000000000..6b70010bc7 --- /dev/null +++ b/acceptance/experimental/aitools/skills/install/test.toml @@ -0,0 +1,57 @@ +# 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. +[[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"] + } + }, + "experimental_skills": { + "test-exp": { + "version": "0.0.1", + "description": "Experimental test skill", + "files": ["SKILL.md"] + } + } +} +''' + +# 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/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 53285f6ffc..2362569816 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -23,14 +23,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 flattened manifest contains +// at least one entry marked Experimental=true. +func manifestHasExperimental(m *Manifest) bool { + for _, meta := range m.Skills { + if meta.Experimental { + 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. If DATABRICKS_SKILLS_REF // is set, it returns that value; otherwise it returns the default ref. @@ -41,11 +67,28 @@ func GetSkillsRef(ctx context.Context) string { return defaultSkillsRepoRef } +// 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 are flattened into Skills during FetchManifest so the +// rest of the installer can treat them uniformly; experimental entries carry +// Experimental=true and RepoDir=experimentalRepoPath. type Manifest struct { - Version string `json:"version"` - UpdatedAt string `json:"updated_at"` - Skills map[string]SkillMeta `json:"skills"` + Version string `json:"version"` + UpdatedAt string `json:"updated_at"` + Skills map[string]SkillMeta `json:"skills"` + ExperimentalSkills map[string]SkillMeta `json:"experimental_skills,omitempty"` } // SkillMeta describes a single skill entry in the manifest. @@ -56,6 +99,19 @@ type SkillMeta struct { Experimental bool `json:"experimental,omitempty"` 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. Populated after manifest parsing; not + // part of the wire format. + RepoDir string `json:"-"` + + // 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. + SourceName string `json:"-"` } // InstallOptions controls the behavior of InstallSkillsForAgents. @@ -65,9 +121,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 { @@ -98,6 +157,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_skills manifest section. + 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 @@ -156,6 +221,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 { @@ -166,7 +248,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 } } @@ -365,9 +447,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 } @@ -465,7 +547,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) @@ -476,7 +558,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/experimental/aitools/lib/installer/installer_test.go b/experimental/aitools/lib/installer/installer_test.go index b769143906..71f07e72ab 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -29,6 +29,7 @@ func (m *mockManifestSource) FetchManifest(_ context.Context, _ string) (*Manife if m.fetchErr != nil { return nil, m.fetchErr } + flattenManifest(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 } } @@ -383,7 +384,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 } @@ -420,7 +421,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 } @@ -504,7 +505,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 } @@ -709,6 +710,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", + ExperimentalSkills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.2.0", Files: []string{"SKILL.md"}}, + }, + } + 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/experimental/aitools/lib/installer/source.go b/experimental/aitools/lib/installer/source.go index e601b26d66..8cdc94c143 100644 --- a/experimental/aitools/lib/installer/source.go +++ b/experimental/aitools/lib/installer/source.go @@ -22,8 +22,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 { @@ -46,5 +45,32 @@ func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (* return nil, fmt.Errorf("failed to parse manifest: %w", err) } + flattenManifest(&manifest) return &manifest, nil } + +// flattenManifest merges ExperimentalSkills into Skills and stamps each entry +// with its RepoDir + SourceName so the rest of the installer can treat both +// kinds uniformly. +// +// Every experimental skill's manifest key gets a "-experimental" suffix so +// the on-disk install path is unambiguously separate from any stable skill +// of the same name. SourceName preserves the upstream repo directory name +// (which does not carry the suffix) for fetch URLs. +func flattenManifest(m *Manifest) { + if m.Skills == nil { + m.Skills = map[string]SkillMeta{} + } + for name, meta := range m.Skills { + meta.RepoDir = stableSkillsRepoPath + meta.SourceName = name + m.Skills[name] = meta + } + for name, meta := range m.ExperimentalSkills { + meta.Experimental = true + meta.RepoDir = experimentalRepoPath + meta.SourceName = name + m.Skills[name+"-experimental"] = meta + } + m.ExperimentalSkills = nil +} diff --git a/experimental/aitools/lib/installer/source_test.go b/experimental/aitools/lib/installer/source_test.go new file mode 100644 index 0000000000..6af847979d --- /dev/null +++ b/experimental/aitools/lib/installer/source_test.go @@ -0,0 +1,104 @@ +package installer + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestFlattenManifestStampsRepoDirAndExperimental(t *testing.T) { + m := &Manifest{ + Skills: map[string]SkillMeta{ + "databricks-apps": {Version: "0.1.0", Files: []string{"SKILL.md"}}, + }, + ExperimentalSkills: map[string]SkillMeta{ + "databricks-iceberg": {Version: "0.0.1", Files: []string{"SKILL.md"}}, + }, + } + + flattenManifest(m) + + assert.Nil(t, m.ExperimentalSkills, "experimental_skills should be moved into skills") + + stable := m.Skills["databricks-apps"] + assert.False(t, stable.Experimental) + assert.Equal(t, stableSkillsRepoPath, stable.RepoDir) + 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.Experimental) + 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 TestFlattenManifestCollidingNamesCoexist(t *testing.T) { + m := &Manifest{ + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.1.0"}, + }, + ExperimentalSkills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.0.1", Files: []string{"SKILL.md"}}, + }, + } + + flattenManifest(m) + + stable := m.Skills["databricks-jobs"] + assert.False(t, stable.Experimental, "stable entry keeps the un-suffixed key") + assert.Equal(t, "0.1.0", stable.Version) + assert.Equal(t, "databricks-jobs", stable.SourceName) + + exp, ok := m.Skills["databricks-jobs-experimental"] + assert.True(t, ok, "experimental copy lives under the -experimental key") + assert.True(t, exp.Experimental) + assert.Equal(t, "0.0.1", exp.Version) + assert.Equal(t, experimentalRepoPath, exp.RepoDir) + assert.Equal(t, "databricks-jobs", exp.SourceName, "SourceName is the un-suffixed name for the fetch URL") +} + +func TestManifestHasExperimental(t *testing.T) { + stableOnly := &Manifest{Skills: map[string]SkillMeta{ + "databricks-apps": {Version: "0.1.0"}, + }} + flattenManifest(stableOnly) + assert.False(t, manifestHasExperimental(stableOnly)) + + withExperimental := &Manifest{ + Skills: map[string]SkillMeta{ + "databricks-apps": {Version: "0.1.0"}, + }, + ExperimentalSkills: map[string]SkillMeta{ + "databricks-iceberg": {Version: "0.0.1"}, + }, + } + flattenManifest(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 TestFlattenManifestEmptyStableSkills(t *testing.T) { + m := &Manifest{ + ExperimentalSkills: map[string]SkillMeta{ + "x": {Version: "0.0.1"}, + }, + } + + flattenManifest(m) + + x, ok := m.Skills["x-experimental"] + assert.True(t, ok, "always-suffix applies even when there is no stable skill of the same name") + assert.True(t, x.Experimental) + assert.Equal(t, experimentalRepoPath, x.RepoDir) + assert.Equal(t, "x", x.SourceName) +} diff --git a/experimental/aitools/lib/installer/uninstall.go b/experimental/aitools/lib/installer/uninstall.go index 1ad9f58511..36e772d3ce 100644 --- a/experimental/aitools/lib/installer/uninstall.go +++ b/experimental/aitools/lib/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/experimental/aitools/lib/installer/uninstall_test.go b/experimental/aitools/lib/installer/uninstall_test.go index 6c7589f6f2..91393c25bb 100644 --- a/experimental/aitools/lib/installer/uninstall_test.go +++ b/experimental/aitools/lib/installer/uninstall_test.go @@ -320,6 +320,71 @@ 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). + tmp := setupTestHome(t) + ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) + setupFetchMock(t) + agent := testAgent(tmp) + + manifest := &Manifest{ + Version: "1", + Skills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.1.0", Files: []string{"SKILL.md"}}, + }, + ExperimentalSkills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.0.1", Files: []string{"SKILL.md"}}, + }, + } + 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")) + require.DirExists(t, filepath.Join(globalDir, "databricks-jobs-experimental")) + + // 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", + ExperimentalSkills: map[string]SkillMeta{ + "databricks-jobs": {Version: "0.0.1", Files: []string{"SKILL.md"}}, + }, + } + 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/experimental/aitools/lib/installer/update.go b/experimental/aitools/lib/installer/update.go index 663ad5e908..a6222ccd5a 100644 --- a/experimental/aitools/lib/installer/update.go +++ b/experimental/aitools/lib/installer/update.go @@ -174,7 +174,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/experimental/aitools/lib/installer/update_test.go b/experimental/aitools/lib/installer/update_test.go index 97e3014be6..09772c8eba 100644 --- a/experimental/aitools/lib/installer/update_test.go +++ b/experimental/aitools/lib/installer/update_test.go @@ -129,7 +129,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 } @@ -165,7 +165,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 }