From 1e7902cfac528cbec1405d9b53f0d60aecffb50b Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 08:29:38 +0100 Subject: [PATCH 1/2] Add install state, manifest source interface, and rename skills directory Add state types (InstallState) with atomic load/save persistence, path resolution (GlobalSkillsDir, ProjectSkillsDir stub), ManifestSource interface with GitHub implementation, latest release discovery (FetchLatestRelease), and Clock abstraction. Extend SkillMeta with v2 fields (Experimental, Description, MinCLIVer). Refactor FetchManifest to delegate to GitHubManifestSource. Rename canonical skills directory from .databricks/agent-skills to .databricks/aitools/skills with backward-compat check for the old path. Co-authored-by: Isaac --- experimental/aitools/lib/agents/skills.go | 12 +- .../aitools/lib/agents/skills_test.go | 22 ++++ .../aitools/lib/installer/installer.go | 39 ++---- experimental/aitools/lib/installer/source.go | 112 ++++++++++++++++++ experimental/aitools/lib/installer/state.go | 100 ++++++++++++++++ .../aitools/lib/installer/state_test.go | 78 ++++++++++++ 6 files changed, 329 insertions(+), 34 deletions(-) create mode 100644 experimental/aitools/lib/installer/source.go create mode 100644 experimental/aitools/lib/installer/state.go create mode 100644 experimental/aitools/lib/installer/state_test.go diff --git a/experimental/aitools/lib/agents/skills.go b/experimental/aitools/lib/agents/skills.go index 3c0d67dfbb..0ba522e356 100644 --- a/experimental/aitools/lib/agents/skills.go +++ b/experimental/aitools/lib/agents/skills.go @@ -14,12 +14,15 @@ const ( databricksSkillPrefix = "databricks" // CanonicalSkillsDir is the shared location for skills when multiple agents are detected. - CanonicalSkillsDir = ".databricks/agent-skills" + CanonicalSkillsDir = ".databricks/aitools/skills" + + // legacySkillsDir is the old canonical location, checked for backward compatibility. + legacySkillsDir = ".databricks/agent-skills" ) // HasDatabricksSkillsInstalled checks if Databricks skills are installed in the canonical location. -// Returns true if no agents are detected (nothing to recommend) or if skills exist in ~/.databricks/agent-skills/. -// Only the canonical location is checked so that skills installed by other tools are not mistaken for a proper installation. +// Returns true if no agents are detected (nothing to recommend) or if skills exist in +// ~/.databricks/aitools/skills/ or the legacy ~/.databricks/agent-skills/. func HasDatabricksSkillsInstalled(ctx context.Context) bool { installed := DetectInstalled(ctx) if len(installed) == 0 { @@ -30,7 +33,8 @@ func HasDatabricksSkillsInstalled(ctx context.Context) bool { if err != nil { return false } - return hasDatabricksSkillsIn(filepath.Join(homeDir, CanonicalSkillsDir)) + return hasDatabricksSkillsIn(filepath.Join(homeDir, CanonicalSkillsDir)) || + hasDatabricksSkillsIn(filepath.Join(homeDir, legacySkillsDir)) } // hasDatabricksSkillsIn checks if dir contains a subdirectory starting with "databricks". diff --git a/experimental/aitools/lib/agents/skills_test.go b/experimental/aitools/lib/agents/skills_test.go index 15baba9c8f..09192d721c 100644 --- a/experimental/aitools/lib/agents/skills_test.go +++ b/experimental/aitools/lib/agents/skills_test.go @@ -134,3 +134,25 @@ func TestHasDatabricksSkillsInstalledDatabricksAppsCanonical(t *testing.T) { assert.True(t, HasDatabricksSkillsInstalled(t.Context())) } + +func TestHasDatabricksSkillsInstalledLegacyPath(t *testing.T) { + tmpHome := t.TempDir() + t.Setenv("HOME", tmpHome) + // Skills only in the legacy location should still be detected. + require.NoError(t, os.MkdirAll(filepath.Join(tmpHome, legacySkillsDir, "databricks"), 0o755)) + + agentDir := filepath.Join(tmpHome, ".claude") + require.NoError(t, os.MkdirAll(agentDir, 0o755)) + + origRegistry := Registry + Registry = []Agent{ + { + Name: "test-agent", + DisplayName: "Test Agent", + ConfigDir: func(_ context.Context) (string, error) { return agentDir, nil }, + }, + } + defer func() { Registry = origRegistry }() + + assert.True(t, HasDatabricksSkillsInstalled(t.Context())) +} diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 084e2dc973..b9f2d7b899 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -2,7 +2,6 @@ package installer import ( "context" - "encoding/json" "fmt" "io" "net/http" @@ -13,7 +12,6 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" - "github.com/databricks/cli/libs/log" "github.com/fatih/color" ) @@ -40,39 +38,20 @@ 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"` + 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"` } // FetchManifest fetches the skills manifest from the skills repo. +// This is a convenience wrapper that uses the default GitHubManifestSource. func FetchManifest(ctx context.Context) (*Manifest, error) { + src := &GitHubManifestSource{} ref := getSkillsRef(ctx) - log.Infof(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) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) - if err != nil { - return nil, fmt.Errorf("failed to create request: %w", err) - } - - client := &http.Client{Timeout: 30 * time.Second} - resp, err := client.Do(req) - if err != nil { - return nil, fmt.Errorf("failed to fetch manifest: %w", err) - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed to fetch manifest: HTTP %d", resp.StatusCode) - } - - var manifest Manifest - if err := json.NewDecoder(resp.Body).Decode(&manifest); err != nil { - return nil, fmt.Errorf("failed to parse manifest: %w", err) - } - - return &manifest, nil + return src.FetchManifest(ctx, ref) } func fetchSkillFile(ctx context.Context, skillName, filePath string) ([]byte, error) { diff --git a/experimental/aitools/lib/installer/source.go b/experimental/aitools/lib/installer/source.go new file mode 100644 index 0000000000..553f04f354 --- /dev/null +++ b/experimental/aitools/lib/installer/source.go @@ -0,0 +1,112 @@ +package installer + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "time" + + "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/log" +) + +// Clock abstracts time for testing. +type Clock interface { + Now() time.Time +} + +// RealClock returns the actual current time. +type RealClock struct{} + +// Now returns the current time. +func (RealClock) Now() time.Time { return time.Now() } + +// ManifestSource abstracts how the skills manifest and release info are fetched. +type ManifestSource interface { + // FetchManifest fetches the skills manifest at the given ref. + FetchManifest(ctx context.Context, ref string) (*Manifest, error) + + // FetchLatestRelease returns the latest release tag. + // Falls back to defaultSkillsRepoRef on any error. + FetchLatestRelease(ctx context.Context) (string, error) +} + +// GitHubManifestSource fetches manifests and release info from GitHub. +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.Infof(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) + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + + client := &http.Client{Timeout: 30 * time.Second} + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("failed to fetch manifest: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("failed to fetch manifest: HTTP %d", resp.StatusCode) + } + + var manifest Manifest + if err := json.NewDecoder(resp.Body).Decode(&manifest); err != nil { + return nil, fmt.Errorf("failed to parse manifest: %w", err) + } + + return &manifest, nil +} + +// FetchLatestRelease returns the latest release tag from GitHub. +// If DATABRICKS_SKILLS_REF is set, it is returned immediately. +// On any error (network, non-200, parse), falls back to defaultSkillsRepoRef. +func (s *GitHubManifestSource) FetchLatestRelease(ctx context.Context) (string, error) { + if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" { + return ref, nil + } + + url := fmt.Sprintf("https://api.github.com/repos/%s/%s/releases/latest", + skillsRepoOwner, skillsRepoName) + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + log.Debugf(ctx, "Failed to create release request, falling back to %s: %v", defaultSkillsRepoRef, err) + return defaultSkillsRepoRef, nil + } + + client := &http.Client{Timeout: 15 * time.Second} + resp, err := client.Do(req) + if err != nil { + log.Debugf(ctx, "Failed to fetch latest release, falling back to %s: %v", defaultSkillsRepoRef, err) + return defaultSkillsRepoRef, nil + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + log.Debugf(ctx, "Latest release returned HTTP %d, falling back to %s", resp.StatusCode, defaultSkillsRepoRef) + return defaultSkillsRepoRef, nil + } + + var release struct { + TagName string `json:"tag_name"` + } + if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { + log.Debugf(ctx, "Failed to parse release response, falling back to %s: %v", defaultSkillsRepoRef, err) + return defaultSkillsRepoRef, nil + } + + if release.TagName == "" { + log.Debugf(ctx, "Empty tag_name in release response, falling back to %s", defaultSkillsRepoRef) + return defaultSkillsRepoRef, nil + } + + return release.TagName, nil +} diff --git a/experimental/aitools/lib/installer/state.go b/experimental/aitools/lib/installer/state.go new file mode 100644 index 0000000000..deb58ef6cf --- /dev/null +++ b/experimental/aitools/lib/installer/state.go @@ -0,0 +1,100 @@ +package installer + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + + "github.com/databricks/cli/libs/env" +) + +const stateFileName = ".state.json" + +// ErrNotImplemented indicates that a feature is not yet implemented. +var ErrNotImplemented = errors.New("project scope not yet implemented") + +// InstalledSkill records the installed version and timestamp for a single skill. +type InstalledSkill struct { + Version string `json:"version"` + InstalledAt string `json:"installed_at"` +} + +// InstallState records the state of all installed skills in a scope directory. +type InstallState struct { + SchemaVersion int `json:"schema_version"` + SkillsRef string `json:"skills_ref"` + LastChecked string `json:"last_checked,omitempty"` + Skills map[string]InstalledSkill `json:"skills"` +} + +// LoadState reads install state from the given directory. +// Returns (nil, nil) when the state file does not exist. +func LoadState(dir string) (*InstallState, error) { + data, err := os.ReadFile(filepath.Join(dir, stateFileName)) + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("failed to read state file: %w", err) + } + + var state InstallState + if err := json.Unmarshal(data, &state); err != nil { + return nil, fmt.Errorf("failed to parse state file: %w", err) + } + return &state, nil +} + +// SaveState writes install state to the given directory atomically. +// Creates the directory if it does not exist. +func SaveState(dir string, state *InstallState) error { + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("failed to create state directory: %w", err) + } + + data, err := json.MarshalIndent(state, "", " ") + if err != nil { + return fmt.Errorf("failed to marshal state: %w", err) + } + + // Atomic write: write to temp file in the same directory, then rename. + tmp, err := os.CreateTemp(dir, ".state-*.tmp") + if err != nil { + return fmt.Errorf("failed to create temp file: %w", err) + } + tmpName := tmp.Name() + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + os.Remove(tmpName) + return fmt.Errorf("failed to write temp file: %w", err) + } + if err := tmp.Close(); err != nil { + os.Remove(tmpName) + return fmt.Errorf("failed to close temp file: %w", err) + } + + if err := os.Rename(tmpName, filepath.Join(dir, stateFileName)); err != nil { + os.Remove(tmpName) + return fmt.Errorf("failed to rename state file: %w", err) + } + return nil +} + +// GlobalSkillsDir returns the path to the global skills directory (~/.databricks/aitools/skills/). +func GlobalSkillsDir(ctx context.Context) (string, error) { + home, err := env.UserHomeDir(ctx) + if err != nil { + return "", err + } + return filepath.Join(home, ".databricks", "aitools", "skills"), nil +} + +// ProjectSkillsDir returns the path to the project-scoped skills directory. +// Project scope is not yet implemented. +func ProjectSkillsDir(_ context.Context) (string, error) { + return "", ErrNotImplemented +} diff --git a/experimental/aitools/lib/installer/state_test.go b/experimental/aitools/lib/installer/state_test.go new file mode 100644 index 0000000000..0d45dff338 --- /dev/null +++ b/experimental/aitools/lib/installer/state_test.go @@ -0,0 +1,78 @@ +package installer + +import ( + "os" + "path/filepath" + "testing" + + "github.com/databricks/cli/libs/env" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLoadStateNonexistentFile(t *testing.T) { + state, err := LoadState(t.TempDir()) + assert.NoError(t, err) + assert.Nil(t, state) +} + +func TestSaveAndLoadStateRoundtrip(t *testing.T) { + dir := t.TempDir() + original := &InstallState{ + SchemaVersion: 1, + SkillsRef: "v0.2.0", + LastChecked: "2026-03-22T10:00:00Z", + Skills: map[string]InstalledSkill{ + "databricks": { + Version: "1.0.0", + InstalledAt: "2026-03-22T09:00:00Z", + }, + }, + } + + err := SaveState(dir, original) + require.NoError(t, err) + + loaded, err := LoadState(dir) + require.NoError(t, err) + assert.Equal(t, original, loaded) +} + +func TestSaveStateCreatesDirectory(t *testing.T) { + dir := filepath.Join(t.TempDir(), "nested", "path") + state := &InstallState{ + SchemaVersion: 1, + SkillsRef: "v0.1.0", + Skills: map[string]InstalledSkill{}, + } + + err := SaveState(dir, state) + require.NoError(t, err) + + // Verify file exists. + _, err = os.Stat(filepath.Join(dir, stateFileName)) + assert.NoError(t, err) +} + +func TestLoadStateCorruptJSON(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, stateFileName), []byte("{bad json"), 0o644)) + + state, err := LoadState(dir) + assert.Error(t, err) + assert.Nil(t, state) + assert.Contains(t, err.Error(), "failed to parse state file") +} + +func TestGlobalSkillsDir(t *testing.T) { + ctx := env.WithUserHomeDir(t.Context(), "/fake/home") + dir, err := GlobalSkillsDir(ctx) + require.NoError(t, err) + assert.Equal(t, filepath.Join("/fake/home", ".databricks", "aitools", "skills"), dir) +} + +func TestProjectSkillsDirNotImplemented(t *testing.T) { + dir, err := ProjectSkillsDir(t.Context()) + assert.ErrorIs(t, err, ErrNotImplemented) + assert.Empty(t, dir) +} From 418a0aee82d5e77da77d3bd67bdf77fe6be0dbd2 Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 22 Mar 2026 08:36:22 +0100 Subject: [PATCH 2/2] Align InstallState with spec, clean up source.go - Rename SkillsRef->Release, LastChecked->LastUpdated (time.Time), simplify Skills to map[string]string, add IncludeExperimental and Scope fields. - Remove unused Clock/RealClock from source.go. - Add trailing newline to SaveState JSON output. - Improve FetchLatestRelease doc comment on error/fallback contract. - Document intentional DATABRICKS_SKILLS_REF duplication. Co-authored-by: Isaac --- experimental/aitools/lib/installer/source.go | 20 +++---- experimental/aitools/lib/installer/state.go | 18 +++--- .../aitools/lib/installer/state_test.go | 56 ++++++++++++++++--- 3 files changed, 63 insertions(+), 31 deletions(-) diff --git a/experimental/aitools/lib/installer/source.go b/experimental/aitools/lib/installer/source.go index 553f04f354..c32eb721d8 100644 --- a/experimental/aitools/lib/installer/source.go +++ b/experimental/aitools/lib/installer/source.go @@ -11,24 +11,16 @@ import ( "github.com/databricks/cli/libs/log" ) -// Clock abstracts time for testing. -type Clock interface { - Now() time.Time -} - -// RealClock returns the actual current time. -type RealClock struct{} - -// Now returns the current time. -func (RealClock) Now() time.Time { return time.Now() } - // ManifestSource abstracts how the skills manifest and release info are fetched. type ManifestSource interface { // FetchManifest fetches the skills manifest at the given ref. FetchManifest(ctx context.Context, ref string) (*Manifest, error) // FetchLatestRelease returns the latest release tag. - // Falls back to defaultSkillsRepoRef on any error. + // Implementations should fall back to a default ref on network errors rather + // than returning an error. The error return exists for cases where fallback is + // not possible (e.g., mock implementations in tests that want to simulate hard + // failures). FetchLatestRelease(ctx context.Context) (string, error) } @@ -68,6 +60,10 @@ func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (* // FetchLatestRelease returns the latest release tag from GitHub. // If DATABRICKS_SKILLS_REF is set, it is returned immediately. // On any error (network, non-200, parse), falls back to defaultSkillsRepoRef. +// +// The DATABRICKS_SKILLS_REF check is intentionally duplicated in getSkillsRef() +// because callers may use either the ManifestSource interface directly or the +// convenience FetchManifest wrapper. func (s *GitHubManifestSource) FetchLatestRelease(ctx context.Context) (string, error) { if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" { return ref, nil diff --git a/experimental/aitools/lib/installer/state.go b/experimental/aitools/lib/installer/state.go index deb58ef6cf..ec17db1f08 100644 --- a/experimental/aitools/lib/installer/state.go +++ b/experimental/aitools/lib/installer/state.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "time" "github.com/databricks/cli/libs/env" ) @@ -16,18 +17,14 @@ const stateFileName = ".state.json" // ErrNotImplemented indicates that a feature is not yet implemented. var ErrNotImplemented = errors.New("project scope not yet implemented") -// InstalledSkill records the installed version and timestamp for a single skill. -type InstalledSkill struct { - Version string `json:"version"` - InstalledAt string `json:"installed_at"` -} - // InstallState records the state of all installed skills in a scope directory. type InstallState struct { - SchemaVersion int `json:"schema_version"` - SkillsRef string `json:"skills_ref"` - LastChecked string `json:"last_checked,omitempty"` - Skills map[string]InstalledSkill `json:"skills"` + SchemaVersion int `json:"schema_version"` + IncludeExperimental bool `json:"include_experimental,omitempty"` + Release string `json:"release"` + LastUpdated time.Time `json:"last_updated"` + Skills map[string]string `json:"skills"` + Scope string `json:"scope,omitempty"` } // LoadState reads install state from the given directory. @@ -59,6 +56,7 @@ func SaveState(dir string, state *InstallState) error { if err != nil { return fmt.Errorf("failed to marshal state: %w", err) } + data = append(data, '\n') // Atomic write: write to temp file in the same directory, then rename. tmp, err := os.CreateTemp(dir, ".state-*.tmp") diff --git a/experimental/aitools/lib/installer/state_test.go b/experimental/aitools/lib/installer/state_test.go index 0d45dff338..ea459dfafe 100644 --- a/experimental/aitools/lib/installer/state_test.go +++ b/experimental/aitools/lib/installer/state_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/databricks/cli/libs/env" "github.com/stretchr/testify/assert" @@ -20,13 +21,10 @@ func TestSaveAndLoadStateRoundtrip(t *testing.T) { dir := t.TempDir() original := &InstallState{ SchemaVersion: 1, - SkillsRef: "v0.2.0", - LastChecked: "2026-03-22T10:00:00Z", - Skills: map[string]InstalledSkill{ - "databricks": { - Version: "1.0.0", - InstalledAt: "2026-03-22T09:00:00Z", - }, + Release: "v0.2.0", + LastUpdated: time.Date(2026, 3, 22, 10, 0, 0, 0, time.UTC), + Skills: map[string]string{ + "databricks": "1.0.0", }, } @@ -42,8 +40,9 @@ func TestSaveStateCreatesDirectory(t *testing.T) { dir := filepath.Join(t.TempDir(), "nested", "path") state := &InstallState{ SchemaVersion: 1, - SkillsRef: "v0.1.0", - Skills: map[string]InstalledSkill{}, + Release: "v0.1.0", + LastUpdated: time.Date(2026, 3, 22, 9, 0, 0, 0, time.UTC), + Skills: map[string]string{}, } err := SaveState(dir, state) @@ -54,6 +53,23 @@ func TestSaveStateCreatesDirectory(t *testing.T) { assert.NoError(t, err) } +func TestSaveStateTrailingNewline(t *testing.T) { + dir := t.TempDir() + state := &InstallState{ + SchemaVersion: 1, + Release: "v0.1.0", + LastUpdated: time.Date(2026, 3, 22, 9, 0, 0, 0, time.UTC), + Skills: map[string]string{}, + } + + err := SaveState(dir, state) + require.NoError(t, err) + + data, err := os.ReadFile(filepath.Join(dir, stateFileName)) + require.NoError(t, err) + assert.Equal(t, byte('\n'), data[len(data)-1]) +} + func TestLoadStateCorruptJSON(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, stateFileName), []byte("{bad json"), 0o644)) @@ -76,3 +92,25 @@ func TestProjectSkillsDirNotImplemented(t *testing.T) { assert.ErrorIs(t, err, ErrNotImplemented) assert.Empty(t, dir) } + +func TestSaveAndLoadStateWithOptionalFields(t *testing.T) { + dir := t.TempDir() + original := &InstallState{ + SchemaVersion: 1, + IncludeExperimental: true, + Release: "v0.3.0", + LastUpdated: time.Date(2026, 3, 22, 12, 30, 0, 0, time.UTC), + Skills: map[string]string{ + "databricks": "1.0.0", + "sql-tools": "0.2.0", + }, + Scope: "project", + } + + err := SaveState(dir, original) + require.NoError(t, err) + + loaded, err := LoadState(dir) + require.NoError(t, err) + assert.Equal(t, original, loaded) +}