aitools: parse experimental_skills manifest section#5243
Conversation
Approval status: pending
|
- experimental/README.md: install examples now use the -experimental suffix on the skill name + the --experimental flag (matching the install-path behaviour landed in databricks/cli#5243). Adds a short note explaining why the in-repo dir name and the install dir name differ. - experimental/README.md: drop databricks-model-serving from the collision example (it was removed from this PR earlier). - experimental/README.md: update the (also available as stable skill) note for databricks-jobs to point at the open TODO #1a. - Root README: clarify the suffixed install name in the by-name install example. Co-authored-by: Isaac
- experimental/README.md: install examples now use the -experimental suffix on the skill name + the --experimental flag (matching the install-path behaviour landed in databricks/cli#5243). Adds a short note explaining why the in-repo dir name and the install dir name differ. - experimental/README.md: drop databricks-model-serving from the collision example (it was removed from this PR earlier). - experimental/README.md: update the (also available as stable skill) note for databricks-jobs to point at the open TODO #1a. - Root README: clarify the suffixed install name in the by-name install example. Co-authored-by: Isaac
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
0ab792b to
2994859
Compare
|
👋 @simonfaltum — Claude here on James's behalf. Rebased onto current Diff is the actual content delta: 22 files, +601/-50 (8 modified, 14 new — 4 acceptance test dirs + Ready for review whenever you've got time. Note on testability: this PR's end-to-end behavior still depends on d-a-s #73 merging and a d-a-s release being cut that contains (comment posted by Claude) |
Summary
The skills manifest in
databricks/databricks-agent-skillsis gaining experimental skills sourced from a newexperimental/directory in the repo (see paired d-a-s PR #73, which imports the ai-dev-kit skill catalog intoexperimental/).This wires the parsing through the aitools installer:
Manifest.Skillsis a single map holding both stable and experimental entries; the per-skillrepo_dirfield ("skills" or "experimental") is the source of truth for whether a skill is experimental.SkillMeta.IsExperimental()derives state fromRepoDir.-experimentalsuffix on their install-side key duringnormalizeManifest;SourceNamepreserves the unsuffixed name for fetch URLs.--experimentalflag (already wired incmd/skills.go) now has experimental skills to install; without it,resolveSkillsfilters them out as before.UX
TODOs / caveats for iteration
Partially resolved. The default ref is still the latest stable release tag (sourced fromDATABRICKS_SKILLS_REFpin.experimental/aitools/lib/installer/SKILLS_VERSION); experimental entries won't exist there until d-a-s cuts a release with PR #73 merged. The default ref bump is a follow-up automated by the SKILLS_VERSION file. UX fix shipped in this PR: if--experimentalis passed but the manifest at the resolved ref exposes no experimental skills, a warning is logged pointing users atDATABRICKS_SKILLS_REF=main.Collision handling is naive.Resolved. Every experimental skill gets a-experimentalsuffix on its install-side key duringnormalizeManifest. The manifest key + install dir both carry the suffix; theSourceNamefield onSkillMetapreserves the upstream repo dir name for fetch URLs. Users see at a glance which installed skills are experimental.Also handled: experimental↔stable transitions. If a skill flips its experimental status upstream (the same logical skill changes manifest key),
installremoves the stale variant on disk + state before installing the new one, anduninstallaccepts either variant name (and removes both if both are present). Helper:alternateVariantKey(). Covered by testsTestInstallReplacesAlternateVariant,TestUninstallByEitherVariantRemovesBoth,TestUninstallByAlternateNameWhenOnlyOneVariantInstalled.Resolved.listUX.aitools skills listshows experimental skills with an[experimental]tag in the NAME column (driven bymeta.IsExperimental()). Combined with the TODO Bump gopkg.in/ini.v1 from 1.66.4 to 1.66.5 #2 resolution (-experimentalsuffix in the manifest key), every experimental row reads e.g.databricks-iceberg-experimental [experimental]— slightly redundant but a clear visual anchor. Hide-by-default was considered but rejected: users runninglistare usually looking for what's available, and silently omitting experimental skills makes them un-discoverable.State tracking.Resolved — kept additive semantics.InstallState.IncludeExperimentalrecords what was last requested but is not used to drive retroactive removal. Runninginstallwithout--experimentalleaves previously-installed experimental skills in place. Rationale: (a) users runninginstallare typically adding/updating, not declaring set membership; (b) silently uninstalling things the user previously asked for is surprising; (c) the transition cleanup shipped under TODO Bump gopkg.in/ini.v1 from 1.66.4 to 1.66.5 #2 handles the actual drift case (skill's experimental status flipping upstream). Removal is whatuninstallis for.No acceptance test yet.Resolved. Added acceptance tests underacceptance/experimental/aitools/skills/install*/covering the install flow against a mocked manifest server:--experimentalinstall adds the experimental skill (with-experimentalsuffix per the install-path model) → 2 skills total--experimentalis idempotentinstall --skills <name>) for both stable and experimental--experimentalagainst a manifest with no experimental entries logs a nudgeTo make these reachable, exposed a new env-var override
DATABRICKS_SKILLS_BASE_URLthat overrides the hard-codedraw.githubusercontent.combase URL used byGitHubManifestSource.FetchManifestandfetchSkillFile. Defaults to the canonical URL when unset, so no production behavior change. UpdatedTaskfile.yml'stest-exp-aitoolstask to includeacceptance/experimental/aitools/**.Variants left as follow-up acceptance tests (the structure is now in place):
Resolved — kept current scope. Each command has internally consistent behavior:--experimentalflag scope.install --experimental→ explicit opt-in (required to install experimental skills).update→ state-driven (honorsInstallState.IncludeExperimentalfrom the lastinstall). If you opted in once, future updates refresh experimentals; otherwise they're skipped.list→ shows all skills with an[experimental]tag (no filtering — discovery first, opt-in to install).Adding
--experimental/--no-experimentaltoupdatefor one-off overrides was considered but rejected: the natural workflow is to re-runinstall --experimental(or justinstall), which already sets the desired state. Follow-up if real users hit a use case for the override.Manifest shape.Resolved. Replaced the original two-map design (skills+experimental_skills+ a per-skillexperimentalbool) with a singleskillsmap where each entry'srepo_dir("skills"or"experimental") is the source of truth. The cli derives experimental state fromRepoDirviaSkillMeta.IsExperimental(). Collisions between stable and experimental skills with the same repo dir name must be resolved upstream in d-a-s (which they already are — d-a-s PR Add bricks command string to user agent #73's TODO #1a merged the only known collision into stable). The d-a-s manifest generator should be updated to emitrepo_dirper skill; until thennormalizeManifestdefaults a missingRepoDirto"skills"so older manifests still parse.Test plan
go build ./...passes.go test ./experimental/aitools/...passes (source_test.gocovers the normalize/IsExperimental cases).go test ./acceptance -run TestAccept/experimental/aitoolspasses (a pre-existing flake intermittently surfaces anlstatwarning during copyDir, ~10% of multi-test runs; unrelated to this refactor)../task lintand./task fmtbefore merge.repo_dir, verify the four UX cases above behave correctly.This pull request and its description were written by Claude.