[1/5] Aitools: state tracking, manifest source, directory rename#4810
[1/5] Aitools: state tracking, manifest source, directory rename#4810simonfaltum wants to merge 6 commits intomainfrom
Conversation
…tory 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
- 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
Suggested reviewersBased on git history of the changed files, these people are best suited to review:
Confidence: high Eligible reviewersBased on CODEOWNERS, these people or teams could also review: @databricks/eng-apps-devex, @lennartkats-db Suggestions based on git history of 6 changed files (3 scored). See CODEOWNERS for path-specific ownership rules. |
arsenyinfo
left a comment
There was a problem hiding this comment.
From my understanding, after these changes we're always pulling latest skills rather than pinned version - is it intentional?
Other than that, given the new canonical path, I'd advocate for migration mechanism when installer is triggered
|
re Migration: I don't think we should be adding migration for the directory rename. This is an experimental feature with a small number of users who opted in. HasDatabricksSkillsInstalled() already detects both old and new paths, so we can surface a "please reinstall" message when we detect the old path. I don't think we really need a migration at this stage |
Skills version is pinned at CLI build time via defaultSkillsRepoRef. No need to call the GitHub releases API for version discovery. Co-authored-by: Isaac
PR Stack
This is part of the aitools feature-complete initiative. PRs should be reviewed and merged in order.
Manifest v2 PR: (pending in databricks-agent-skills)
Why
The aitools skills installer has no state tracking. There's no record of what was installed, at what version, or when. This blocks every lifecycle command (update, uninstall, version). The manifest fetching logic is also hardcoded with no abstraction for testing.
Changes
Before: Skills install to
~/.databricks/agent-skills/with no state file.FetchManifestis inlined ininstaller.gowith a hardcoded ref and no way to mock it in tests.Now:
InstallStatetype withLoadState/SaveState(atomic write via temp+rename) for tracking installed skills, release ref, and timestampsManifestSourceinterface withGitHubManifestSource, separating manifest fetching from install logic. Enables test mocking.FetchLatestReleaseto discover newest release from GitHub API. Falls back to pinned ref on network failure.SkillMeta(Experimental,Description,MinCLIVer) with omitempty for backward compat with v1 manifests.databricks/agent-skillsto.databricks/aitools/skillsHasDatabricksSkillsInstalledfor the old pathNo behavior changes to existing commands except the directory path.
Test plan
LoadStatereturns nil, nil for nonexistent fileSaveState+LoadStateroundtrip preserves all fields including optional onesSaveStatecreates nested directories, writes trailing newlineLoadStatereturns error for corrupt JSONGlobalSkillsDirresolves correctlyProjectSkillsDirreturns ErrNotImplementedHasDatabricksSkillsInstalleddetects legacy path