[4/5] Aitools: list command, flat structure, --skills/--agents/--include-experimental flags#4813
Merged
simonfaltum merged 10 commits intomainfrom Mar 26, 2026
Merged
Conversation
This was referenced Mar 22, 2026
9 tasks
8d6f603 to
14680ad
Compare
f2fba9a to
1243dc4
Compare
14680ad to
2dd4a43
Compare
1243dc4 to
ccf24e9
Compare
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 12 changed files (5 scored). See CODEOWNERS for path-specific ownership rules. |
2dd4a43 to
7cc1b0c
Compare
ccf24e9 to
3a9003f
Compare
7cc1b0c to
c917624
Compare
3a9003f to
50babdc
Compare
arsenyinfo
requested changes
Mar 24, 2026
Contributor
arsenyinfo
left a comment
There was a problem hiding this comment.
nitpicker's output but looks legit:
Critical Data Integrity Issues
- Deduplicate --skills for Uninstall: The removeAll logic compares len(toRemove) to the total installed skills. Providing duplicate names (e.g., --skills foo,foo) triggers an incorrect state wipe, deleting the state file while leaving other installed skill files orphaned on disk.
- Deduplicate --agents for Install: Duplicate agents in the flag cause the installer to incorrectly switch to "symlink mode" (thinking multiple agents are targeted) and perform redundant file operations.
UX & Functional Regressions
- Whitespace Trimming: The --skills flag lacks whitespace trimming after splitting by commas. Inputs like --skills "skill1, skill2" will fail to find " skill2".
- Broken Backward Compatibility: The hidden skills install alias is missing the --experimental flag, which will break existing user scripts.
- Positional Argument Silent Failures: The install, update, uninstall, list, and version commands ignore positional arguments (e.g., aitools install databricks). These should enforce cobra.NoArgs to prevent users from thinking they are targeting a specific skill when they are actually triggering a global operation.
- Inconsistent Flag Types: The --skills flag is a string on mutation commands but a bool on list/version. This inconsistency creates a jarring CLI experience and should be unified or renamed.
Member
Author
|
Those all look valid. I'll address them and update the PR |
arsenyinfo
approved these changes
Mar 25, 2026
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 25, 2026
## PR Stack This is part of the aitools feature-complete initiative. PRs should be reviewed and merged in order. 1. **[1/5] State + release discovery + directory rename** (this PR) 2. [2/5] Install writes state + interactive agent selection (#4811) 3. [3/5] Update + uninstall + version commands (#4812) 4. [4/5] List improvements + command restructuring + flags (#4813) 5. [5/5] Project scope (--project/--global) (pending) 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. `FetchManifest` is inlined in `installer.go` with a hardcoded ref and no way to mock it in tests. Now: - `InstallState` type with `LoadState`/`SaveState` (atomic write via temp+rename) for tracking installed skills, release ref, and timestamps - `ManifestSource` interface with `GitHubManifestSource`, separating manifest fetching from install logic. Enables test mocking. - `FetchLatestRelease` to discover newest release from GitHub API. Falls back to pinned ref on network failure. - v2 fields on `SkillMeta` (`Experimental`, `Description`, `MinCLIVer`) with omitempty for backward compat with v1 manifests - Canonical directory renamed from `.databricks/agent-skills` to `.databricks/aitools/skills` - Backward-compat check in `HasDatabricksSkillsInstalled` for the old path No behavior changes to existing commands except the directory path. ## Test plan - [x] `LoadState` returns nil, nil for nonexistent file - [x] `SaveState` + `LoadState` roundtrip preserves all fields including optional ones - [x] `SaveState` creates nested directories, writes trailing newline - [x] `LoadState` returns error for corrupt JSON - [x] `GlobalSkillsDir` resolves correctly - [x] `ProjectSkillsDir` returns ErrNotImplemented - [x] `HasDatabricksSkillsInstalled` detects legacy path - [x] All existing aitools tests still pass --------- Co-authored-by: Arseny Kravchenko <arsenyinfo@users.noreply.github.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 25, 2026
…mpotent install (#4811) ## PR Stack 1. [1/5] State + release discovery + directory rename (#4810) 2. **[2/5] Install writes state + interactive agent selection** (this PR) 3. [3/5] Update + uninstall + version commands (#4812) 4. [4/5] List improvements + command restructuring + flags (#4813) 5. [5/5] Project scope (--project/--global) (pending) **Base**: `simonfaltum/aitools-pr1-state` (PR 1) ## Why Install currently has no state tracking, no filtering, and outputs every file download to the console. Users can't tell what version they have, and there's no way to install for specific agents. ## Changes Before: `skills install` downloads everything, prints every file, has no state, no filtering, no agent selection. Now: - `InstallSkillsForAgents(ctx, src, agents, opts)` as the core install function. `InstallAllSkills` becomes a thin wrapper (signature preserved for `cmd/apps/init.go` compat). - State written to `.state.json` after successful install. Merges with existing state (doesn't overwrite). - Idempotent: second install skips already-present skills with matching versions. - Experimental filtering: skip `experimental: true` skills by default. - `min_cli_version` enforcement: skip incompatible skills with warning (hard error for single-skill install). - Interactive agent selection via `charmbracelet/huh` multi-select. Skip prompt for 1 agent, all detected for non-interactive. - Legacy install detection: prints "reinstall" message when old install found without state. - Concise default output (2 lines). Per-file detail only at debug level. ## Test plan - [x] State created after install-all and install-single - [x] Experimental filtering (skip by default, include with flag) - [x] min_cli_version: warning for all, hard error for single - [x] Idempotent: skip same version, update changed - [x] Legacy detection with helpful message - [x] Interactive prompt for 2+ agents, skip for 1 - [x] Non-interactive: all detected, no prompt - [x] `InstallAllSkills` signature preserved - [x] Concise output, per-file at debug level
c917624 to
0023f1b
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 25, 2026
## PR Stack 1. [1/5] State + release discovery + directory rename (#4810) 2. [2/5] Install writes state + interactive agent selection (#4811) 3. **[3/5] Update + uninstall + version commands** (this PR) 4. [4/5] List improvements + command restructuring + flags (#4813) 5. [5/5] Project scope (--project/--global) (pending) **Base**: `simonfaltum/aitools-pr2-install` (PR 2) ## Why Users can install skills but can't update, uninstall, or check what version they have. These are core lifecycle gaps. ## Changes Three new commands, each small but grouped because they share patterns. **`aitools update`**: Compares installed release against latest. Downloads changed skills. Flags: `--check` (dry run), `--force` (re-download), `--no-new` (don't auto-add new manifest skills). Authoritative release detection (distinguishes real API response from fallback). Applies experimental and min_cli_version filtering. Warns on skills removed from manifest. **`aitools uninstall`**: Removes skill directories from canonical location. Removes symlinks from ALL agent directories (full registry scan, not just detected). Cleans orphaned symlinks. Only removes symlinks pointing to canonical dir (safe for third-party skills). Deletes `.state.json` on success. **`aitools version`**: Shows installed version, skill count, last updated date. Best-effort staleness check against latest release. Graceful offline degradation. Suppresses staleness check when `DATABRICKS_SKILLS_REF` is set. ## Test plan - [x] Update: no state -> error, already up to date, version diff, check dry run, force, no-new, new skill auto-installed, removed skill warning - [x] Uninstall: removes dirs + symlinks, orphan cleanup, state deletion, no state -> error, missing dirs handled - [x] Version: installed/up-to-date, update available, not installed, offline graceful - [x] All lint checks pass
Add --skills, --agents, and --include-experimental flags to install command. Add --skills flag to update, uninstall, and version commands. Create new list command with detailed table output showing available/installed skills. Make skills subcommand hidden for backward compat while promoting flat command structure. Add selective uninstall support via UninstallOptions. Co-authored-by: Isaac
…ning - Always call SetArgs in skills install backward-compat alias (fixes Execute path inheriting parent args) - Add cobra.MaximumNArgs(1) to reject extra positional args - Remove dead installer.ListSkills function (replaced by list command) - Restore yellow color for "No agents detected" warning in install.go - Add Execute-path tests for skills install alias Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
FetchLatestRelease was removed in the PR3 squash-merge. Use GetSkillsRef(ctx) to get the skills repo ref instead. Co-authored-by: Isaac
48d9fca to
e0166f7
Compare
Collaborator
|
Commit: 113448b |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 26, 2026
## PR Stack 1. [1/5] State + release discovery + directory rename (#4810) 2. [2/5] Install writes state + interactive agent selection (#4811) 3. [3/5] Update + uninstall + version commands (#4812) 4. [4/5] List improvements + command restructuring + flags (#4813) 5. **[5/5] Project scope (--project/--global)** (this PR) Manifest v2 PR: databricks/databricks-agent-skills#35 **Base**: `simonfaltum/aitools-pr4-restructure` (PR 4) ## Why Skills are currently global-only. Teams working on the same project can't share a curated set of skills via their repo. Project-scoped installation puts skills alongside the code, so they can be committed to git and shared. ## Changes Before: All skills install to `~/.databricks/aitools/skills/` (global only). Now: - `--project` flag on install, update, uninstall: operates on `<cwd>/.databricks/aitools/skills/` - `--global` flag: explicit global scope (default behavior) - `--global` + `--project` -> error - Interactive scope prompt on install (default: global). Uses `huh.NewSelect` in cmd layer. - Non-interactive defaults to global. - `SupportsProjectScope` field on `Agent` struct. Only Claude Code and Cursor support project scope. - Incompatible agents get a warning, not an error: "Skipped <agent>: does not support project-scoped skills." - Version shows both scopes when both exist. - State includes `scope: "project"` field for project installs. ## Test plan - [x] `--project` installs to cwd-relative directory - [x] `--global` + `--project` -> error - [x] No flag, interactive -> scope prompt shown, default is global - [x] No flag, non-interactive -> global (no prompt) - [x] Incompatible agents warned, not errored - [x] Symlinks only created for capable agents in project scope - [x] Version shows both scopes when both exist - [x] `SupportsProjectScope` set correctly for all 6 agents - [x] All lint checks pass
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Stack
Manifest v2 PR: databricks/databricks-agent-skills#35
Base:
simonfaltum/aitools-pr3-lifecycle(PR 3)Why
Commands are nested under
skillswhich burns namespace for future component types (hooks, etc.). There's no way to install specific skills, target specific agents from the CLI, or see installed status in the list output.Changes
New
listcommand: Table output with skill names, versions, installed status,[experimental]tags. Sorted alphabetically. Usestabwriterfor alignment.Flat command structure:
aitools install/update/uninstall/list/versionat the top level. Hiddenskills installandskills listaliases for backward compat.Flags:
--skills(string, comma-separated) on install, update, uninstall: operate on specific skills--skills(bool) on list, version: show detailed skills view--agents(string, comma-separated) on install: target specific agents, validates against registry, skips interactive prompt--include-experimentalon install: include experimental skillsSelective uninstall:
--skillson uninstall removes only named skills, preserves state file with remaining.Test plan
install --skills,--agents,--include-experimentalflags work--agentsvalidates against registry, errors on unknownuninstall --skillsselective removal, state preservedupdate --skillsselective updatelistshows detailed tableskills installandskills listhidden aliases work (Execute path tested)cmd/apps/init.gointegration preserved