Promote aitools skills-management commands to top-level#4917
Promote aitools skills-management commands to top-level#4917jamesbroadhead wants to merge 7 commits intomainfrom
Conversation
Approval status: pending
|
|
I think we should move the code itself (the files) out of experimental as part of this as well. |
Per Simon's review on #4917, the source files should live outside experimental/ now that aitools is a top-level command. Renames the directory from experimental/aitools/ to aitools/ and updates all import paths, examples, and tooling references (Makefile, OWNERS, testmask, pr-checklist). The experimental/aitools alias still routes to the same package via its new import path, so 'databricks experimental aitools' continues to work as a deprecated entry point. Co-authored-by: Isaac
c1b87a6 to
8b230d7
Compare
|
No findings. 🔍 Reviewed by nitpicker |
Move install/update/uninstall/list/version source files (and the agents/installer libs they depend on) from experimental/aitools/ to a new top-level aitools/ package. Wire `databricks aitools <X>` at top-level. The tools subtree (query, discover-schema, get-default-warehouse, statement) and the hidden skills alias group stay under experimental/aitools/ where their stability story still lives. The old paths under `databricks experimental aitools install/update/uninstall/list/version` and `databricks experimental aitools skills install/list` keep working as silent backward-compat aliases (no deprecation warnings). Co-authored-by: Isaac
User-facing strings that pointed callers at `databricks experimental aitools install/update` now point at the new canonical `databricks aitools install/update`. Test assertions updated to match. Co-authored-by: Isaac
The README and PR description advertise `databricks aitools install [skill-name]`, but the promoted command had `Use: "install"` and `cobra.NoArgs`, so the documented form would fail. Match the docs: accept an optional positional skill name and map it to the same path as `--skills`. The two are mutually exclusive to keep behavior unambiguous. Replace the old reject-positional-args test with coverage for the positional path and the conflict case. Co-authored-by: Isaac
This reverts commit 99a6273.
The README listed `databricks aitools install [skill-name]`, but the command is `Use: "install"` with `cobra.NoArgs` and uses `--skills <name>` for specific skills. Match the actual command. Co-authored-by: Isaac
a9f9f5e to
7efa0d3
Compare
|
Hi @lennartkats-db — Claude here, on James's behalf. This PR has changed shape since you were originally tagged. A heads-up before you take a look: Simon is now effectively a co-author. He force-pushed and rewrote the branch on Apr 30 — the original commits are gone. The current commits implement the "promote skills surface, keep tools experimental" approach: source files for install/update/uninstall/list/version physically moved to a new Branch is now rebased onto current main and includes a couple of follow-up fixes from a self-review pass:
CI: all standard checks green. Integration Tests show one failure (https://go/deco-tests/25155597272) — the changes here shouldn't be touching anything integration-relevant, so it reads as a flake, but worth a quick triage if you happen to be looking. |
After the silent-alias restructure, install/update/uninstall/list/version and the skills alias group are all Hidden under `experimental aitools`, leaving only `tools` visible. The Long string still advertised install and skills as if they were active here. Reword to: state that skills-management has moved to top-level, and that `tools` is the only experimental surface still maintained under this path. Co-authored-by: Isaac
e8711d8 to
e7f09a4
Compare
Restores the positional skill-name form on the canonical command. The legacy alias `databricks experimental aitools skills install <name>` already accepted a positional and forwarded it via --skills; the canonical `databricks aitools install <name>` previously rejected positionals (cobra.NoArgs), so users mechanically migrating scripts hit "unknown command". Behavior: - `databricks aitools install` — install all skills (unchanged) - `databricks aitools install <name>` — install just that skill - `databricks aitools install --skills a,b` — install multiple - positional + --skills together — error, mutually exclusive Replaces TestInstallRejectsPositionalArgs with positive coverage for the positional path, the conflict case, and the >1-positional rejection. Co-authored-by: Isaac
Summary
databricks aitools:install,update,uninstall,list,versionexperimental/aitools/to a new top-levelaitools/package, so directory layout matches stability tiertoolssubtree underexperimental/aitools/— query, discover-schema, get-default-warehouse, statement — becausetools.gostill says "There are no stability guarantees for these tools"skillsalias group also stays underexperimental/aitools/databricks experimental aitools install/update/uninstall/list/versionanddatabricks experimental aitools skills install/listkeep working as silent backward-compat aliases — no deprecation warning fires, the commands just forwardThe aitools skills-management surface is feature-complete after the 5-PR series (#4810–#4814) that added state tracking, lifecycle commands, and project scope support. The
toolssubtree is functionally useful but its shape is still in flux, so promoting only the stable half.Command shape after this PR
Test plan
databricks aitools --helpshows install/update/uninstall/list/version (notools)databricks --helplistsaitoolsin the outputdatabricks experimental aitools installruns without warning and the work happens (silent alias)databricks experimental aitools tools query …runs as beforedatabricks experimental aitools tools --helplists query/discover-schema/get-default-warehouse/statementexperimental/aitools/cmd/skills_test.goThis pull request was AI-assisted by Isaac.