From 55c23c7d2373dde71896f8dde30e50f7bd5c160c Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 14:59:29 +0200 Subject: [PATCH 01/10] feat: move cli-compat manifest to CLI repo with 3-tier fallback - Embed cli-compat.json in the CLI binary with build-time fetch - Resolve AppKit and Agent Skills versions from the manifest - Add 1h local cache and retry for runtime manifest fetches - Show default AppKit version in --version flag help - Print resolved skills version during aitools install Co-authored-by: Isaac --- .agent/skills/bump-cli-compat/SKILL.md | 126 ++++++ .github/OWNERS | 4 + cmd/apps/init.go | 16 +- cmd/apps/init_test.go | 2 +- cmd/apps/manifest.go | 7 +- experimental/aitools/cmd/list.go | 5 +- experimental/aitools/cmd/version.go | 6 +- .../aitools/lib/installer/SKILLS_VERSION | 1 - .../aitools/lib/installer/installer.go | 25 +- .../aitools/lib/installer/installer_test.go | 43 +- .../aitools/lib/installer/uninstall_test.go | 2 + experimental/aitools/lib/installer/update.go | 5 +- .../aitools/lib/installer/update_test.go | 12 +- experimental/aitools/lib/installer/version.go | 14 - internal/build/README.md | 55 +++ internal/build/cli-compat.json | 4 + internal/build/dep_versions.go | 9 + libs/depversions/depversions.go | 342 ++++++++++++++ libs/depversions/depversions_test.go | 426 ++++++++++++++++++ 19 files changed, 1058 insertions(+), 46 deletions(-) create mode 100644 .agent/skills/bump-cli-compat/SKILL.md delete mode 100644 experimental/aitools/lib/installer/SKILLS_VERSION delete mode 100644 experimental/aitools/lib/installer/version.go create mode 100644 internal/build/README.md create mode 100644 internal/build/cli-compat.json create mode 100644 internal/build/dep_versions.go create mode 100644 libs/depversions/depversions.go create mode 100644 libs/depversions/depversions_test.go diff --git a/.agent/skills/bump-cli-compat/SKILL.md b/.agent/skills/bump-cli-compat/SKILL.md new file mode 100644 index 0000000000..3256b3f74b --- /dev/null +++ b/.agent/skills/bump-cli-compat/SKILL.md @@ -0,0 +1,126 @@ +--- +name: bump-cli-compat +description: "Bump cli-compat.json with new AppKit and Agent Skills versions, then create a PR. Use when the user says 'bump cli-compat', 'update cli-compat', 'bump compatibility manifest', 'new appkit release cli-compat', or wants to update the CLI compatibility manifest after an AppKit or Agent Skills release." +user-invocable: true +allowed-tools: Read, Edit, Write, Bash, Glob, Grep, AskUserQuestion +--- + +# Bump CLI Compatibility Manifest + +Updates `internal/build/cli-compat.json` with new AppKit and Agent Skills versions, validates the result, and creates a PR. + +## Arguments + +Parse the user's input for optional version arguments: + +- `--appkit ` or first positional arg → AppKit version (e.g. `0.28.0`) +- `--skills ` or second positional arg → Agent Skills version (e.g. `0.1.6`) +- No args → auto-detect latest versions from GitHub tags + +Versions should be provided **without** the `v` prefix (e.g. `0.28.0`, not `v0.28.0`). If provided with the prefix, strip it. + +## Workflow + +### Step 1: Resolve versions + +If both `appkit` and `skills` versions were provided as arguments, skip to Step 2. + +Otherwise, fetch the latest tags from GitHub: + +```bash +# Latest appkit version (strip leading 'v') +gh api repos/databricks/appkit/tags --jq '.[0].name' | sed 's/^v//' + +# Latest skills version (strip leading 'v') +gh api repos/databricks/databricks-agent-skills/tags --jq '.[0].name' | sed 's/^v//' +``` + +Show the resolved versions to the user and ask: + +> The latest versions are: +> - AppKit: `{appkit_version}` +> - Agent Skills: `{skills_version}` +> +> Have these versions been evaluated (evals passed with no regressions)? + +**Do NOT proceed until the user confirms.** If the user says no or wants different versions, ask them to provide the correct versions. + +### Step 2: Validate tags exist + +Verify that the corresponding Git tags exist on GitHub: + +```bash +gh api repos/databricks/appkit/git/ref/tags/v{appkit_version} --jq '.ref' 2>&1 +gh api repos/databricks/databricks-agent-skills/git/ref/tags/v{skills_version} --jq '.ref' 2>&1 +``` + +If either tag doesn't exist, report the error and stop. + +### Step 3: Read current manifest + +Read `internal/build/cli-compat.json`. Note the current versions and the list of versioned entries. + +### Step 4: Update the manifest + +Update **all entries** (both `next` and all versioned CLI entries) to the new appkit and skills versions. This is the "no template changes" scenario — a simple search & replace. + +Write the updated `internal/build/cli-compat.json`. + +### Step 5: Validate + +Run the Go tests to ensure the manifest is well-formed: + +```bash +go test ./libs/depversions/... -run TestEmbeddedManifest -v +``` + +If validation fails, show the errors and fix them before proceeding. + +### Step 6: Create branch, commit, and PR + +```bash +# Create a new branch from the current branch (or main) +git checkout -b bump-cli-compat-appkit-{appkit_version}-skills-{skills_version} + +# Stage and commit +git add internal/build/cli-compat.json +git commit -s -m "chore: bump cli-compat to appkit {appkit_version}, skills {skills_version}" + +# Push and create PR +git push -u origin HEAD +gh pr create \ + --title "chore: bump cli-compat to appkit {appkit_version}, skills {skills_version}" \ + --body "$(cat <<'EOF' +## Summary +Bump `cli-compat.json` to use: +- AppKit `{appkit_version}` +- Agent Skills `{skills_version}` + +## Checklist +- [ ] Evals passed with no regressions +- [ ] `go test ./libs/depversions/... -run TestEmbeddedManifest` passes +EOF +)" +``` + +Show the PR URL to the user when done. + +## Examples + +### Example: With explicit versions +``` +/bump-cli-compat 0.28.0 0.1.6 +``` +Validates tags exist, updates manifest, creates PR. + +### Example: Auto-detect latest +``` +/bump-cli-compat +``` +Fetches latest tags, asks for eval confirmation, then updates and creates PR. + +### Example: With flags +``` +/bump-cli-compat --appkit 0.28.0 --skills 0.1.6 +``` +Same as positional args. diff --git a/.github/OWNERS b/.github/OWNERS index 7cae525465..91084750e8 100644 --- a/.github/OWNERS +++ b/.github/OWNERS @@ -59,5 +59,9 @@ # Internal /internal/ team:platform +# CLI compatibility manifest +/internal/build/cli-compat.json team:eng-apps-devex team:platform +/libs/depversions/ team:eng-apps-devex team:platform + # Experimental /experimental/aitools/ team:eng-apps-devex @lennartkats-db diff --git a/cmd/apps/init.go b/cmd/apps/init.go index 2f1da99bb7..5daba0e202 100644 --- a/cmd/apps/init.go +++ b/cmd/apps/init.go @@ -25,6 +25,7 @@ import ( "github.com/databricks/cli/libs/apps/prompt" "github.com/databricks/cli/libs/cmdctx" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/depversions" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/log" @@ -38,7 +39,6 @@ const ( appkitTemplateDir = "template" appkitDefaultBranch = "main" appkitTemplateTagPfx = "template-v" - appkitDefaultVersion = "template-v0.24.0" defaultProfile = "DEFAULT" ) @@ -169,7 +169,11 @@ Environment variables: cmd.Flags().StringVar(&templatePath, "template", "", "Template path (local directory or GitHub URL)") cmd.Flags().StringVar(&branch, "branch", "", "Git branch or tag (for GitHub templates, mutually exclusive with --version)") - cmd.Flags().StringVar(&version, "version", "", fmt.Sprintf("AppKit version to use (default: %s, use 'latest' for main branch)", appkitDefaultVersion)) + versionDesc := "AppKit version to use (use 'latest' for main branch)" + if v := depversions.EmbeddedDefaultAppKitVersion(); v != "" { + versionDesc = fmt.Sprintf("AppKit version to use (default: %s, use 'latest' for main branch)", v) + } + cmd.Flags().StringVar(&version, "version", "", versionDesc) cmd.Flags().StringVar(&name, "name", "", "Project name (prompts if not provided)") cmd.Flags().StringVar(&warehouseID, "warehouse-id", "", "SQL warehouse ID") _ = cmd.Flags().MarkDeprecated("warehouse-id", "use --set .sql-warehouse.id= instead") @@ -805,8 +809,12 @@ func runCreate(ctx context.Context, opts createOptions) error { case opts.version != "": gitRef = normalizeVersion(opts.version) default: - // Default: use pinned version - gitRef = appkitDefaultVersion + appkitVersion, err := depversions.ResolveAppKitVersion(ctx) + if err != nil { + return fmt.Errorf("could not resolve AppKit template version: %w. Use --version to specify a version manually", err) + } + gitRef = normalizeVersion(appkitVersion) + cmdio.LogString(ctx, "Using AppKit template version "+appkitVersion) } templateSrc = appkitRepoURL } diff --git a/cmd/apps/init_test.go b/cmd/apps/init_test.go index 81e71eed3f..fe406dfb7b 100644 --- a/cmd/apps/init_test.go +++ b/cmd/apps/init_test.go @@ -443,7 +443,7 @@ func TestNormalizeVersion(t *testing.T) { {"", ""}, {"main", "main"}, {"feat/something", "feat/something"}, - {appkitDefaultVersion, appkitDefaultVersion}, + {"template-v0.24.0", "template-v0.24.0"}, } for _, tt := range tests { diff --git a/cmd/apps/manifest.go b/cmd/apps/manifest.go index 38df201acc..ff200e6998 100644 --- a/cmd/apps/manifest.go +++ b/cmd/apps/manifest.go @@ -9,6 +9,7 @@ import ( "path/filepath" "github.com/databricks/cli/libs/apps/manifest" + "github.com/databricks/cli/libs/depversions" "github.com/databricks/cli/libs/env" "github.com/spf13/cobra" ) @@ -27,7 +28,11 @@ func runManifestOnly(ctx context.Context, templatePath, branch, version string) case version != "": gitRef = normalizeVersion(version) default: - gitRef = appkitDefaultVersion + appkitVersion, err := depversions.ResolveAppKitVersion(ctx) + if err != nil { + return fmt.Errorf("could not resolve AppKit template version: %w. Use --version to specify a version manually", err) + } + gitRef = normalizeVersion(appkitVersion) } templateSrc = appkitRepoURL } diff --git a/experimental/aitools/cmd/list.go b/experimental/aitools/cmd/list.go index 1be1538c9a..8f665d7ad1 100644 --- a/experimental/aitools/cmd/list.go +++ b/experimental/aitools/cmd/list.go @@ -48,7 +48,10 @@ func newListCmd() *cobra.Command { func defaultListSkills(cmd *cobra.Command, scope string) error { ctx := cmd.Context() - ref := installer.GetSkillsRef(ctx) + ref, err := installer.GetSkillsRef(ctx) + if err != nil { + return err + } src := &installer.GitHubManifestSource{} manifest, err := src.FetchManifest(ctx, ref) diff --git a/experimental/aitools/cmd/version.go b/experimental/aitools/cmd/version.go index 67c38fec42..17faa468a6 100644 --- a/experimental/aitools/cmd/version.go +++ b/experimental/aitools/cmd/version.go @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/installer" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/log" "github.com/spf13/cobra" ) @@ -44,7 +45,10 @@ func newVersionCmd() *cobra.Command { return nil } - latestRef := installer.GetSkillsRef(ctx) + latestRef, err := installer.GetSkillsRef(ctx) + if err != nil { + log.Debugf(ctx, "could not resolve skills version: %v", err) + } bothScopes := globalState != nil && projectState != nil cmdio.LogString(ctx, "Databricks AI Tools:") diff --git a/experimental/aitools/lib/installer/SKILLS_VERSION b/experimental/aitools/lib/installer/SKILLS_VERSION deleted file mode 100644 index 027a383a35..0000000000 --- a/experimental/aitools/lib/installer/SKILLS_VERSION +++ /dev/null @@ -1 +0,0 @@ -v0.1.5 diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 53285f6ffc..39e04120ba 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -17,6 +17,7 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/depversions" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" "golang.org/x/mod/semver" @@ -32,13 +33,17 @@ const ( // It is a package-level var so tests can replace it with a mock. var fetchFileFn = fetchSkillFile -// GetSkillsRef returns the skills repo ref to use. If DATABRICKS_SKILLS_REF -// is set, it returns that value; otherwise it returns the default ref. -func GetSkillsRef(ctx context.Context) string { +// GetSkillsRef returns the skills repo ref to use. +// Resolution order: DATABRICKS_SKILLS_REF env var → compatibility manifest → error. +func GetSkillsRef(ctx context.Context) (string, error) { if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" { - return ref + return ref, nil } - return defaultSkillsRepoRef + v, err := depversions.ResolveAgentSkillsVersion(ctx) + if err != nil { + return "", fmt.Errorf("could not resolve skills version: %w", err) + } + return "v" + v, nil } // Manifest describes the skills manifest fetched from the skills repo. @@ -92,7 +97,12 @@ func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byt // This is the core installation function. Callers are responsible for agent detection, // prompting, and printing the "Installing..." header. func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgents []*agents.Agent, opts InstallOptions) error { - ref := GetSkillsRef(ctx) + ref, err := GetSkillsRef(ctx) + if err != nil { + return err + } + tag := strings.TrimPrefix(ref, "v") + cmdio.LogString(ctx, "Using skills version "+tag) manifest, err := src.FetchManifest(ctx, ref) if err != nil { return err @@ -193,12 +203,11 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent return err } - tag := strings.TrimPrefix(ref, "v") noun := "skills" if len(targetSkills) == 1 { noun = "skill" } - cmdio.LogString(ctx, fmt.Sprintf("Installed %d %s (v%s).", len(targetSkills), noun, tag)) + cmdio.LogString(ctx, fmt.Sprintf("Installed %d %s.", len(targetSkills), noun)) return nil } diff --git a/experimental/aitools/lib/installer/installer_test.go b/experimental/aitools/lib/installer/installer_test.go index b769143906..df3bf8f6b0 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -3,12 +3,10 @@ package installer import ( "bytes" "context" - "fmt" "io/fs" "log/slog" "os" "path/filepath" - "strings" "testing" "github.com/databricks/cli/experimental/aitools/lib/agents" @@ -19,6 +17,8 @@ import ( "github.com/stretchr/testify/require" ) +const testSkillsRef = "v0.1.5" + // mockManifestSource is a test double for ManifestSource. type mockManifestSource struct { manifest *Manifest @@ -192,6 +192,7 @@ func TestInstallSkillsForAgentsWritesState(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) src := &mockManifestSource{manifest: testManifest()} agent := testAgent(tmp) @@ -204,18 +205,19 @@ func TestInstallSkillsForAgentsWritesState(t *testing.T) { require.NoError(t, err) require.NotNil(t, state) assert.Equal(t, 1, state.SchemaVersion) - assert.Equal(t, defaultSkillsRepoRef, state.Release) + assert.Equal(t, testSkillsRef, state.Release) assert.Len(t, state.Skills, 2) assert.Equal(t, "0.1.0", state.Skills["databricks-sql"]) assert.Equal(t, "0.1.0", state.Skills["databricks-jobs"]) - assert.Contains(t, stderr.String(), fmt.Sprintf("Installed 2 skills (%s).", defaultSkillsRepoRef)) + assert.Contains(t, stderr.String(), "Installed 2 skills.") } func TestInstallSkillForSingleWritesState(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) src := &mockManifestSource{manifest: testManifest()} agent := testAgent(tmp) @@ -232,13 +234,14 @@ func TestInstallSkillForSingleWritesState(t *testing.T) { assert.Len(t, state.Skills, 1) assert.Equal(t, "0.1.0", state.Skills["databricks-sql"]) - assert.Contains(t, stderr.String(), fmt.Sprintf("Installed 1 skill (%s).", defaultSkillsRepoRef)) + assert.Contains(t, stderr.String(), "Installed 1 skill.") } func TestInstallSkillsSpecificNotFound(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) src := &mockManifestSource{manifest: testManifest()} agent := testAgent(tmp) @@ -254,6 +257,7 @@ func TestExperimentalSkillsSkippedByDefault(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) manifest := testManifest() manifest.Skills["databricks-experimental"] = SkillMeta{ @@ -275,13 +279,14 @@ func TestExperimentalSkillsSkippedByDefault(t *testing.T) { assert.Len(t, state.Skills, 2) assert.NotContains(t, state.Skills, "databricks-experimental") - assert.Contains(t, stderr.String(), fmt.Sprintf("Installed 2 skills (%s).", defaultSkillsRepoRef)) + assert.Contains(t, stderr.String(), "Installed 2 skills.") } func TestExperimentalSkillsIncludedWithFlag(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) manifest := testManifest() manifest.Skills["databricks-experimental"] = SkillMeta{ @@ -305,13 +310,14 @@ func TestExperimentalSkillsIncludedWithFlag(t *testing.T) { assert.Contains(t, state.Skills, "databricks-experimental") assert.True(t, state.IncludeExperimental) - assert.Contains(t, stderr.String(), fmt.Sprintf("Installed 3 skills (%s).", defaultSkillsRepoRef)) + assert.Contains(t, stderr.String(), "Installed 3 skills.") } func TestMinCLIVersionSkipWithWarningForInstallAll(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) setBuildVersion(t, "0.200.0") // Capture log output to verify the warning. @@ -339,7 +345,7 @@ func TestMinCLIVersionSkipWithWarningForInstallAll(t *testing.T) { assert.Len(t, state.Skills, 2) assert.NotContains(t, state.Skills, "databricks-future") - assert.Contains(t, stderr.String(), fmt.Sprintf("Installed 2 skills (%s).", defaultSkillsRepoRef)) + assert.Contains(t, stderr.String(), "Installed 2 skills.") assert.Contains(t, logBuf.String(), "requires CLI version 0.300.0") } @@ -347,6 +353,7 @@ func TestMinCLIVersionHardErrorForInstallSingle(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) setBuildVersion(t, "0.200.0") manifest := testManifest() @@ -371,6 +378,7 @@ func TestIdempotentSecondInstallSkips(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) src := &mockManifestSource{manifest: testManifest()} agent := testAgent(tmp) @@ -400,6 +408,7 @@ func TestIdempotentInstallUpdatesNewVersions(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) src := &mockManifestSource{manifest: testManifest()} agent := testAgent(tmp) @@ -437,7 +446,7 @@ func TestIdempotentInstallUpdatesNewVersions(t *testing.T) { globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") state, err := LoadState(globalDir) require.NoError(t, err) - assert.Equal(t, defaultSkillsRepoRef, state.Release) + assert.Equal(t, testSkillsRef, state.Release) assert.Equal(t, "0.2.0", state.Skills["databricks-sql"]) } @@ -445,6 +454,7 @@ func TestLegacyDetectMessagePrinted(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Create skills on disk at canonical location but no state file. globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") @@ -463,6 +473,7 @@ func TestLegacyDetectLegacyDir(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Create skills in the legacy location. legacyDir := filepath.Join(tmp, ".databricks", "agent-skills") @@ -481,6 +492,7 @@ func TestIdempotentInstallReinstallsForNewAgent(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) src := &mockManifestSource{manifest: testManifest()} agent1 := testAgent(tmp) @@ -526,6 +538,7 @@ func TestLegacyTargetedInstallBlocked(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Create skills on disk at canonical location but no state file (legacy). globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") @@ -546,6 +559,7 @@ func TestLegacyFullInstallAllowed(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Create skills on disk at canonical location but no state file (legacy). globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") @@ -590,6 +604,7 @@ func TestInstallProjectScopeWritesState(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Use project dir as cwd. projectDir := filepath.Join(tmp, "myproject") @@ -607,17 +622,17 @@ func TestInstallProjectScopeWritesState(t *testing.T) { require.NoError(t, err) require.NotNil(t, state) assert.Equal(t, ScopeProject, state.Scope) - assert.Equal(t, defaultSkillsRepoRef, state.Release) + assert.Equal(t, testSkillsRef, state.Release) assert.Len(t, state.Skills, 2) - tag := strings.TrimPrefix(defaultSkillsRepoRef, "v") - assert.Contains(t, stderr.String(), fmt.Sprintf("Installed 2 skills (v%s).", tag)) + assert.Contains(t, stderr.String(), "Installed 2 skills.") } func TestInstallProjectScopeCreatesSymlinks(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) projectDir := filepath.Join(tmp, "myproject") require.NoError(t, os.MkdirAll(projectDir, 0o755)) @@ -660,6 +675,7 @@ func TestInstallProjectScopeFiltersIncompatibleAgents(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) projectDir := filepath.Join(tmp, "myproject") require.NoError(t, os.MkdirAll(projectDir, 0o755)) @@ -680,13 +696,14 @@ func TestInstallProjectScopeFiltersIncompatibleAgents(t *testing.T) { require.NoError(t, err) assert.Contains(t, stderr.String(), "Skipped No Project Agent: does not support project-scoped skills.") - assert.Contains(t, stderr.String(), fmt.Sprintf("Installed 2 skills (%s).", defaultSkillsRepoRef)) + assert.Contains(t, stderr.String(), "Installed 2 skills.") } func TestInstallProjectScopeZeroCompatibleAgentsReturnsError(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) projectDir := filepath.Join(tmp, "myproject") require.NoError(t, os.MkdirAll(projectDir, 0o755)) diff --git a/experimental/aitools/lib/installer/uninstall_test.go b/experimental/aitools/lib/installer/uninstall_test.go index 6c7589f6f2..444fb773af 100644 --- a/experimental/aitools/lib/installer/uninstall_test.go +++ b/experimental/aitools/lib/installer/uninstall_test.go @@ -18,6 +18,7 @@ func installTestSkills(t *testing.T, tmp string) string { t.Helper() ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) src := &mockManifestSource{manifest: testManifest()} agent := testAgent(tmp) @@ -48,6 +49,7 @@ func TestUninstallRemovesSymlinks(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Use two registry-based agents so uninstall can find them. // Create config dirs for claude-code and cursor (both in agents.Registry). diff --git a/experimental/aitools/lib/installer/update.go b/experimental/aitools/lib/installer/update.go index 663ad5e908..0aa260f115 100644 --- a/experimental/aitools/lib/installer/update.go +++ b/experimental/aitools/lib/installer/update.go @@ -81,7 +81,10 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent return nil, errors.New("no skills installed. Run 'databricks experimental aitools install' to install") } - latestTag := GetSkillsRef(ctx) + latestTag, err := GetSkillsRef(ctx) + if err != nil { + return nil, err + } if state.Release == latestTag && !opts.Force { cmdio.LogString(ctx, "Already up to date.") diff --git a/experimental/aitools/lib/installer/update_test.go b/experimental/aitools/lib/installer/update_test.go index 97e3014be6..3272a77bab 100644 --- a/experimental/aitools/lib/installer/update_test.go +++ b/experimental/aitools/lib/installer/update_test.go @@ -46,6 +46,7 @@ func TestUpdateAlreadyUpToDate(t *testing.T) { tmp := setupTestHome(t) ctx, stderr := cmdio.NewTestContextWithStderr(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Install first. src := &mockManifestSource{manifest: testManifest()} @@ -68,6 +69,7 @@ func TestUpdateVersionDiffDetected(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Install with default ref. src := &mockManifestSource{manifest: testManifest()} @@ -108,6 +110,7 @@ func TestUpdateCheckDryRun(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Install with default ref. src := &mockManifestSource{manifest: testManifest()} @@ -148,13 +151,14 @@ func TestUpdateCheckDryRun(t *testing.T) { globalDir := filepath.Join(tmp, ".databricks", "aitools", "skills") state, err := LoadState(globalDir) require.NoError(t, err) - assert.Equal(t, defaultSkillsRepoRef, state.Release) + assert.Equal(t, testSkillsRef, state.Release) } func TestUpdateForceRedownloads(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Install v0.1.0. src := &mockManifestSource{manifest: testManifest()} @@ -182,6 +186,7 @@ func TestUpdateAutoAddsNewSkills(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Install with default ref. src := &mockManifestSource{manifest: testManifest()} @@ -218,6 +223,7 @@ func TestUpdateNoNewIgnoresNewSkills(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Install with default ref. src := &mockManifestSource{manifest: testManifest()} @@ -254,6 +260,7 @@ func TestUpdateOutputSortedAlphabetically(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Install with skills. src := &mockManifestSource{manifest: testManifest()} @@ -281,6 +288,7 @@ func TestUpdateSkillRemovedFromManifestWarning(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Capture log output to verify warning. var logBuf bytes.Buffer @@ -325,6 +333,7 @@ func TestUpdateSkipsExperimentalSkills(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) // Install with default ref (not experimental). src := &mockManifestSource{manifest: testManifest()} @@ -355,6 +364,7 @@ func TestUpdateSkipsMinCLIVersionSkills(t *testing.T) { tmp := setupTestHome(t) ctx := cmdio.MockDiscard(t.Context()) setupFetchMock(t) + t.Setenv("DATABRICKS_SKILLS_REF", testSkillsRef) setBuildVersion(t, "0.200.0") var logBuf bytes.Buffer diff --git a/experimental/aitools/lib/installer/version.go b/experimental/aitools/lib/installer/version.go deleted file mode 100644 index 0e942ca0a9..0000000000 --- a/experimental/aitools/lib/installer/version.go +++ /dev/null @@ -1,14 +0,0 @@ -package installer - -import ( - _ "embed" - "strings" -) - -//go:embed SKILLS_VERSION -var skillsVersionFile string - -// defaultSkillsRepoRef is the pinned tag of databricks/databricks-agent-skills. -// It is sourced from the SKILLS_VERSION file so automation can bump the pin -// with a single-line file edit instead of patching Go source. -var defaultSkillsRepoRef = strings.TrimSpace(skillsVersionFile) diff --git a/internal/build/README.md b/internal/build/README.md new file mode 100644 index 0000000000..71409737c7 --- /dev/null +++ b/internal/build/README.md @@ -0,0 +1,55 @@ +# CLI Compatibility Manifest + +`cli-compat.json` maps Databricks CLI versions to compatible AppKit and Agent Skills versions. The CLI uses this manifest to determine which template version to use for `apps init` and which skills version to use for `aitools install`. + +## Manifest format + +```json +{ + "next": { "appkit": "0.24.0", "skills": "0.1.4" }, + "0.299.0": { "appkit": "0.24.0", "skills": "0.1.4" } +} +``` + +- Each key is a CLI version (`X.Y.Z`) or `"next"`. +- Each value specifies the compatible `appkit` and `skills` versions. +- `"next"` is used for CLI versions newer than any listed entry and for dev builds. + +## How the CLI resolves versions + +1. **Exact match** on CLI version → use that entry. +2. **No exact match**, between two entries → use the nearest lower version's entry. +3. **Newer than all entries** → use the highest versioned entry. +4. **Older than all entries** → use the lowest (oldest) entry. +5. **Dev builds** (`0.0.0-dev*`) → use `"next"`. + +## Manifest sources (fallback chain) + +At runtime, the CLI resolves the manifest from three sources: + +1. **Local cache** (`~/.cache/databricks/compat-manifest.json`) — used if fresh (< 1 hour old). +2. **Remote fetch** from GitHub — used when cache is stale or missing. On success, the local cache is updated. +3. **Stale local cache** — if remote fetch fails but a previously cached file exists (even if expired), it is used as-is. +4. **Embedded manifest** — compiled into the binary via `go:embed`. Used as last resort when both remote and local cache fail. + +## When to update + +After each AppKit release: + +1. **Run evals** on the new AppKit version. If there is no regression, proceed. +2. **Open a PR** to update `cli-compat.json`. The change depends on the type of release: + - **No template changes** (just an AppKit/skills version bump): search & replace all version occurrences in the manifest and update `next`. + - **Template changes that don't require new CLI features**: test the last 3 CLI versions with the new template and update matching entries. + - **Template changes that require new CLI features**: add a new entry for the minimum CLI version that supports them; older entries keep pointing to the previous template version. + +This process is manual for now but can be automated as part of the release workflow in the future. Use the `/bump-cli-compat` Claude Code skill to automate the update and PR creation. + +## Validation + +The manifest is validated by Go tests in `libs/depversions/`: + +```bash +go test ./libs/depversions/... -run TestEmbeddedManifest -v +``` + +This checks: valid JSON, `"next"` key present, at least one versioned entry, valid semver keys, `next` versions >= all entries, and ascending key order. diff --git a/internal/build/cli-compat.json b/internal/build/cli-compat.json new file mode 100644 index 0000000000..b7cff3b831 --- /dev/null +++ b/internal/build/cli-compat.json @@ -0,0 +1,4 @@ +{ + "next": { "appkit": "0.24.0", "skills": "0.1.4" }, + "0.299.0": { "appkit": "0.24.0", "skills": "0.1.4" } +} diff --git a/internal/build/dep_versions.go b/internal/build/dep_versions.go new file mode 100644 index 0000000000..c5bc84635c --- /dev/null +++ b/internal/build/dep_versions.go @@ -0,0 +1,9 @@ +package build + +import _ "embed" + +// EmbeddedManifestJSON is the cli-compat.json manifest embedded at compile time. +// Used as the last-resort fallback when both remote fetch and local cache fail. +// +//go:embed cli-compat.json +var EmbeddedManifestJSON []byte diff --git a/libs/depversions/depversions.go b/libs/depversions/depversions.go new file mode 100644 index 0000000000..da2febc59e --- /dev/null +++ b/libs/depversions/depversions.go @@ -0,0 +1,342 @@ +package depversions + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "slices" + "strings" + "time" + + "github.com/databricks/cli/internal/build" + "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/log" + "golang.org/x/mod/semver" +) + +const ( + // manifestURL is the raw GitHub URL for the compatibility manifest. + manifestURL = "https://raw.githubusercontent.com/databricks/cli/main/internal/build/cli-compat.json" + + // fetchTimeout is the HTTP timeout for fetching the manifest at runtime. + fetchTimeout = 3 * time.Second + + // nextKey is the special manifest key for CLI versions newer than any entry. + nextKey = "next" + + // cacheTTL is how long a locally cached manifest is considered fresh. + cacheTTL = 1 * time.Hour + + // localManifestFile is the filename for the locally cached manifest. + localManifestFile = "compat-manifest.json" + + fetchRetries = 2 + fetchRetryBackoff = 300 * time.Millisecond +) + +// Entry maps a CLI version to compatible AppKit and Agent Skills versions. +type Entry struct { + AppKit string `json:"appkit"` + AgentSkills string `json:"skills"` +} + +// Manifest is the compatibility manifest: a map of CLI version strings to entries. +type Manifest map[string]Entry + +// cachedManifest holds a parsed manifest together with its on-disk mod time. +type cachedManifest struct { + manifest Manifest + modTime time.Time +} + +// isFresh reports whether the cached manifest is younger than maxAge. +func (c cachedManifest) isFresh(maxAge time.Duration) bool { + return time.Since(c.modTime) < maxAge +} + +// httpClient is the HTTP client used for manifest fetches. Package-level var +// so tests can replace it. +var httpClient = &http.Client{Timeout: fetchTimeout} + +// FetchManifest returns the compatibility manifest using a 3-tier strategy: +// 1. Local cached file (if fresh, < 1 hour old) +// 2. Remote fetch from GitHub (with retry) +// 3. Stale local file (if remote fails but a previously cached file exists) +// 4. Embedded manifest compiled into the binary +func FetchManifest(ctx context.Context) (Manifest, error) { + localPath := manifestLocalPath(ctx) + + // Read local file once — reuse across tiers. + local, localErr := readLocalManifest(localPath) + + // Tier 1: local file is fresh. + if localErr == nil && local.isFresh(cacheTTL) { + log.Debugf(ctx, "Using cached manifest from %s", localPath) + return local.manifest, nil + } + + // Tier 2: fetch from remote (local file missing or stale). + m, fetchErr := fetchRemoteWithRetry(ctx) + if fetchErr == nil { + writeLocalManifest(ctx, localPath, m) + return m, nil + } + + // Tier 3a: local file exists but stale — use it anyway. + if localErr == nil { + log.Debugf(ctx, "Using stale cached manifest (remote failed: %v)", fetchErr) + return local.manifest, nil + } + + // Tier 3b: embedded manifest. + if m, err := parseManifest(build.EmbeddedManifestJSON); err == nil { + log.Debugf(ctx, "Using embedded manifest (remote and local cache failed)") + return m, nil + } + + return nil, fmt.Errorf("all manifest sources failed: %w", fetchErr) +} + +// EmbeddedDefaultAppKitVersion returns the "next" entry's AppKit version from +// the embedded manifest. Used for help text defaults where a network call is +// not appropriate. Returns "" if the embedded manifest is invalid. +func EmbeddedDefaultAppKitVersion() string { + m, err := parseManifest(build.EmbeddedManifestJSON) + if err != nil { + return "" + } + if next, ok := m[nextKey]; ok { + return next.AppKit + } + return "" +} + +// Resolve returns the manifest entry for the given CLI version. +// +// Resolution order: +// 1. Dev builds (version starts with "0.0.0-dev") use the "next" entry. +// 2. Exact match on CLI version. +// 3. Nearest lower version (semver-sorted). This also handles CLI versions +// newer than all entries, returning the highest known entry. +// 4. If CLI is older than all entries, use the lowest (oldest) entry. +func Resolve(m Manifest, cliVersion string) (Entry, error) { + if len(m) == 0 { + return Entry{}, errors.New("empty compatibility manifest") + } + + next, ok := m[nextKey] + if !ok { + return Entry{}, fmt.Errorf("compatibility manifest missing %q key", nextKey) + } + + // Dev builds always use "next". + if strings.HasPrefix(cliVersion, "0.0.0-dev") { + return next, nil + } + + // Exact match. + if entry, ok := m[cliVersion]; ok { + return entry, nil + } + + // Collect and sort versioned keys (exclude "next"). + var versions []string + for k := range m { + if k != nextKey { + versions = append(versions, k) + } + } + + // Sort descending by semver. The semver package requires a "v" prefix. + slices.SortFunc(versions, func(a, b string) int { + return semver.Compare("v"+b, "v"+a) + }) + + // Find the nearest lower version. + vCLI := "v" + cliVersion + for _, v := range versions { + if semver.Compare("v"+v, vCLI) <= 0 { + return m[v], nil + } + } + + // CLI is older than all entries — use the lowest (oldest) entry as closest match. + // If there are no versioned entries (only "next"), fall back to "next". + if len(versions) == 0 { + return next, nil + } + return m[versions[len(versions)-1]], nil +} + +// resolveEntry fetches the manifest, resolves for the given CLI version. +func resolveEntry(ctx context.Context, cliVersion string) (Entry, error) { + m, err := FetchManifest(ctx) + if err != nil { + return Entry{}, err + } + return Resolve(m, cliVersion) +} + +// ResolveAppKitVersion resolves the AppKit template version for the current CLI. +func ResolveAppKitVersion(ctx context.Context) (string, error) { + entry, err := resolveEntry(ctx, build.GetInfo().Version) + if err != nil { + return "", err + } + return entry.AppKit, nil +} + +// ResolveAgentSkillsVersion resolves the Agent Skills version for the current CLI. +func ResolveAgentSkillsVersion(ctx context.Context) (string, error) { + entry, err := resolveEntry(ctx, build.GetInfo().Version) + if err != nil { + return "", err + } + return entry.AgentSkills, nil +} + +// --- Local manifest cache --- + +// manifestLocalPath returns the path to the locally cached manifest file. +func manifestLocalPath(ctx context.Context) string { + if dir := env.Get(ctx, "DATABRICKS_CACHE_DIR"); dir != "" { + return filepath.Join(dir, localManifestFile) + } + home, err := os.UserCacheDir() + if err != nil { + return "" + } + return filepath.Join(home, "databricks", localManifestFile) +} + +// readLocalManifest reads and parses the locally cached manifest file. +func readLocalManifest(path string) (cachedManifest, error) { + if path == "" { + return cachedManifest{}, errors.New("no cache path") + } + info, err := os.Stat(path) + if err != nil { + return cachedManifest{}, err + } + data, err := os.ReadFile(path) + if err != nil { + return cachedManifest{}, err + } + m, err := parseManifest(data) + if err != nil { + return cachedManifest{}, err + } + return cachedManifest{manifest: m, modTime: info.ModTime()}, nil +} + +// writeLocalManifest atomically writes the manifest to the local cache path. +func writeLocalManifest(ctx context.Context, path string, m Manifest) { + if path == "" { + return + } + data, err := json.Marshal(m) + if err != nil { + log.Debugf(ctx, "Failed to marshal manifest for cache: %v", err) + return + } + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { + log.Debugf(ctx, "Failed to create cache directory %s: %v", dir, err) + return + } + tmp, err := os.CreateTemp(dir, ".compat-manifest-*.tmp") + if err != nil { + log.Debugf(ctx, "Failed to create temp cache file: %v", err) + return + } + tmpPath := tmp.Name() + defer func() { + _ = tmp.Close() + _ = os.Remove(tmpPath) + }() + if _, err := tmp.Write(data); err != nil { + log.Debugf(ctx, "Failed to write temp cache file: %v", err) + return + } + if err := tmp.Close(); err != nil { + log.Debugf(ctx, "Failed to close temp cache file: %v", err) + return + } + _ = os.Remove(path) + if err := os.Rename(tmpPath, path); err != nil { + log.Debugf(ctx, "Failed to rename temp cache file: %v", err) + } +} + +// --- Remote fetch --- + +// fetchRemoteWithRetry wraps fetchRemote with retries. +func fetchRemoteWithRetry(ctx context.Context) (Manifest, error) { + var lastErr error + for attempt := range fetchRetries + 1 { + if attempt > 0 { + log.Debugf(ctx, "Retrying manifest fetch (attempt %d)", attempt+1) + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(fetchRetryBackoff): + } + } + m, err := fetchRemote(ctx) + if err == nil { + return m, nil + } + lastErr = err + } + return nil, lastErr +} + +func fetchRemote(ctx context.Context) (Manifest, error) { + log.Debugf(ctx, "Fetching compatibility manifest from %s", manifestURL) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, manifestURL, nil) + if err != nil { + return nil, err + } + + resp, err := httpClient.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("HTTP %d fetching manifest", resp.StatusCode) + } + + // Cap response size to guard against corrupted or malicious responses. + body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + return nil, err + } + + return parseManifest(body) +} + +func parseManifest(data []byte) (Manifest, error) { + var m Manifest + if err := json.Unmarshal(data, &m); err != nil { + return nil, fmt.Errorf("invalid manifest JSON: %w", err) + } + if len(m) == 0 { + return nil, errors.New("empty compatibility manifest") + } + if _, ok := m[nextKey]; !ok { + return nil, fmt.Errorf("compatibility manifest missing %q key", nextKey) + } + for k := range m { + if k != nextKey && !semver.IsValid("v"+k) { + return nil, fmt.Errorf("invalid semver key %q in compatibility manifest", k) + } + } + return m, nil +} diff --git a/libs/depversions/depversions_test.go b/libs/depversions/depversions_test.go new file mode 100644 index 0000000000..73be235ad6 --- /dev/null +++ b/libs/depversions/depversions_test.go @@ -0,0 +1,426 @@ +package depversions + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "sync/atomic" + "testing" + "time" + + "github.com/databricks/cli/internal/build" + "github.com/databricks/cli/libs/env" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/mod/semver" +) + +// roundTripFunc adapts a function into an http.RoundTripper. +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + +// redirectToServer replaces the package-level httpClient with one whose +// transport rewrites every request URL to point at srv. +func redirectToServer(t *testing.T, srv *httptest.Server) { + t.Helper() + orig := httpClient + httpClient = &http.Client{ + Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { + target, _ := url.Parse(srv.URL) + req.URL.Scheme = target.Scheme + req.URL.Host = target.Host + return http.DefaultTransport.RoundTrip(req) + }), + } + t.Cleanup(func() { httpClient = orig }) +} + +// testContext returns a context with an isolated cache directory so tests don't +// share cached manifests. +func testContext(t *testing.T) context.Context { + t.Helper() + return env.Set(t.Context(), "DATABRICKS_CACHE_DIR", t.TempDir()) +} + +func testManifest() Manifest { + return Manifest{ + "next": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, + "0.296.0": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, + "0.295.0": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, + "0.290.0": {AppKit: "0.24.0", AgentSkills: "0.1.5"}, + "0.288.0": {AppKit: "0.24.0", AgentSkills: "0.1.4"}, + } +} + +// --- Resolve tests (unchanged, no network) --- + +func TestResolve_ExactMatch(t *testing.T) { + m := testManifest() + entry, err := Resolve(m, "0.296.0") + require.NoError(t, err) + assert.Equal(t, "0.27.0", entry.AppKit) + assert.Equal(t, "0.1.5", entry.AgentSkills) +} + +func TestResolve_NearestLower(t *testing.T) { + m := testManifest() + entry, err := Resolve(m, "0.293.0") + require.NoError(t, err) + assert.Equal(t, "0.24.0", entry.AppKit) + assert.Equal(t, "0.1.5", entry.AgentSkills) +} + +func TestResolve_NewerThanAll(t *testing.T) { + m := Manifest{ + "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + "0.296.0": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, + "0.290.0": {AppKit: "0.24.0", AgentSkills: "0.1.5"}, + } + entry, err := Resolve(m, "0.300.0") + require.NoError(t, err) + assert.Equal(t, "0.27.0", entry.AppKit) + assert.Equal(t, "0.1.5", entry.AgentSkills) +} + +func TestResolve_DevBuild(t *testing.T) { + m := testManifest() + entry, err := Resolve(m, "0.0.0-dev+abc123def") + require.NoError(t, err) + assert.Equal(t, "0.27.0", entry.AppKit) + assert.Equal(t, "0.1.5", entry.AgentSkills) +} + +func TestResolve_OlderThanAll(t *testing.T) { + m := testManifest() + entry, err := Resolve(m, "0.280.0") + require.NoError(t, err) + assert.Equal(t, "0.24.0", entry.AppKit) + assert.Equal(t, "0.1.4", entry.AgentSkills) +} + +func TestResolve_OnlyNextKey(t *testing.T) { + m := Manifest{ + "next": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, + } + entry, err := Resolve(m, "0.296.0") + require.NoError(t, err) + assert.Equal(t, "0.27.0", entry.AppKit) + assert.Equal(t, "0.1.5", entry.AgentSkills) +} + +func TestResolve_LowestEntryExactMatch(t *testing.T) { + m := testManifest() + entry, err := Resolve(m, "0.288.0") + require.NoError(t, err) + assert.Equal(t, "0.24.0", entry.AppKit) + assert.Equal(t, "0.1.4", entry.AgentSkills) +} + +func TestResolve_EmptyManifest(t *testing.T) { + m := Manifest{} + _, err := Resolve(m, "0.296.0") + assert.Error(t, err) + assert.Contains(t, err.Error(), "empty compatibility manifest") +} + +func TestResolve_MissingNextKey(t *testing.T) { + m := Manifest{ + "0.296.0": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, + } + _, err := Resolve(m, "0.296.0") + assert.Error(t, err) + assert.Contains(t, err.Error(), `missing "next" key`) +} + +// --- FetchManifest tests --- + +func TestFetchManifest_RemoteSuccess(t *testing.T) { + ctx := testContext(t) + want := Manifest{ + "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + } + body, _ := json.Marshal(want) + + var called bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + _, _ = w.Write(body) + })) + defer srv.Close() + redirectToServer(t, srv) + + result, err := FetchManifest(ctx) + require.NoError(t, err) + assert.True(t, called, "test server should have been called") + assert.Equal(t, "0.99.0", result["next"].AppKit) +} + +func TestFetchManifest_RemoteFailFallsBackToEmbedded(t *testing.T) { + ctx := testContext(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + redirectToServer(t, srv) + + // No local cache exists, so should fall back to embedded manifest. + result, err := FetchManifest(ctx) + require.NoError(t, err) + + // Verify it returned the embedded manifest values. + embedded, _ := parseManifest(build.EmbeddedManifestJSON) + assert.Equal(t, embedded["next"].AppKit, result["next"].AppKit) +} + +func TestFetchManifest_RemoteFailFallsBackToStaleCache(t *testing.T) { + ctx := testContext(t) + + // Pre-populate the local cache with a stale manifest. + staleManifest := Manifest{ + "next": {AppKit: "0.88.0", AgentSkills: "0.8.8"}, + "0.296.0": {AppKit: "0.88.0", AgentSkills: "0.8.8"}, + } + localPath := manifestLocalPath(ctx) + writeLocalManifest(ctx, localPath, staleManifest) + // Make it stale by setting mod time to 2 hours ago. + past := time.Now().Add(-2 * time.Hour) + require.NoError(t, os.Chtimes(localPath, past, past)) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + redirectToServer(t, srv) + + result, err := FetchManifest(ctx) + require.NoError(t, err) + // Should return the stale cached manifest, not the embedded one. + assert.Equal(t, "0.88.0", result["next"].AppKit) +} + +func TestFetchManifest_RemoteSuccessWritesLocalCache(t *testing.T) { + ctx := testContext(t) + want := Manifest{ + "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + } + body, _ := json.Marshal(want) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write(body) + })) + defer srv.Close() + redirectToServer(t, srv) + + _, err := FetchManifest(ctx) + require.NoError(t, err) + + // Verify the local cache was written. + localPath := manifestLocalPath(ctx) + _, statErr := os.Stat(localPath) + assert.NoError(t, statErr, "local cache file should exist after successful fetch") +} + +func TestFetchManifest_CacheHit(t *testing.T) { + ctx := testContext(t) + want := Manifest{ + "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + } + body, _ := json.Marshal(want) + + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + _, _ = w.Write(body) + })) + defer srv.Close() + redirectToServer(t, srv) + + // First call: populates cache. + result1, err := FetchManifest(ctx) + require.NoError(t, err) + assert.Equal(t, "0.99.0", result1["next"].AppKit) + + // Second call: should come from cache, not hitting the server again. + result2, err := FetchManifest(ctx) + require.NoError(t, err) + assert.Equal(t, "0.99.0", result2["next"].AppKit) + + assert.Equal(t, int32(1), callCount.Load(), "server should only be called once; second call should be a cache hit") +} + +func TestFetchManifest_RetryOnTransientError(t *testing.T) { + ctx := testContext(t) + want := Manifest{ + "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + } + body, _ := json.Marshal(want) + + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := callCount.Add(1) + if n == 1 { + w.WriteHeader(http.StatusInternalServerError) + return + } + _, _ = w.Write(body) + })) + defer srv.Close() + redirectToServer(t, srv) + + result, err := FetchManifest(ctx) + require.NoError(t, err) + assert.Equal(t, "0.99.0", result["next"].AppKit) + assert.Equal(t, int32(2), callCount.Load(), "should have retried after first failure") +} + +// --- parseManifest tests --- + +func TestParseManifest_Valid(t *testing.T) { + data := `{"next":{"appkit":"0.27.0","skills":"0.1.5"},"0.296.0":{"appkit":"0.27.0","skills":"0.1.5"}}` + m, err := parseManifest([]byte(data)) + require.NoError(t, err) + assert.Equal(t, "0.27.0", m["next"].AppKit) + assert.Equal(t, "0.27.0", m["0.296.0"].AppKit) +} + +func TestParseManifest_InvalidJSON(t *testing.T) { + _, err := parseManifest([]byte("not json")) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid manifest JSON") +} + +func TestParseManifest_MissingNext(t *testing.T) { + data := `{"0.296.0":{"appkit":"0.27.0","skills":"0.1.5"}}` + _, err := parseManifest([]byte(data)) + assert.Error(t, err) + assert.Contains(t, err.Error(), `missing "next" key`) +} + +func TestParseManifest_Empty(t *testing.T) { + _, err := parseManifest([]byte("{}")) + assert.Error(t, err) + assert.Contains(t, err.Error(), "empty compatibility manifest") +} + +// --- resolveEntry tests --- + +func TestResolveEntry_RemoteSuccess(t *testing.T) { + ctx := testContext(t) + want := Manifest{ + "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + } + body, _ := json.Marshal(want) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write(body) + })) + defer srv.Close() + redirectToServer(t, srv) + + entry, err := resolveEntry(ctx, "0.296.0") + require.NoError(t, err) + assert.Equal(t, "0.99.0", entry.AppKit) + assert.Equal(t, "0.9.9", entry.AgentSkills) +} + +func TestResolveEntry_RemoteFailUsesEmbedded(t *testing.T) { + ctx := testContext(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + redirectToServer(t, srv) + + // Should succeed via the embedded manifest fallback. + entry, err := resolveEntry(ctx, "0.0.0-dev+test") + require.NoError(t, err) + assert.NotEmpty(t, entry.AppKit) +} + +// --- EmbeddedDefaultAppKitVersion --- + +func TestEmbeddedDefaultAppKitVersion(t *testing.T) { + v := EmbeddedDefaultAppKitVersion() + assert.NotEmpty(t, v, "embedded manifest should have a next.appkit version") + assert.True(t, semver.IsValid("v"+v), "embedded default version should be valid semver") +} + +// --- Embedded manifest validation (replaces AppKit TS validator) --- + +func TestEmbeddedManifest_IsWellFormed(t *testing.T) { + m, err := parseManifest(build.EmbeddedManifestJSON) + require.NoError(t, err, "embedded manifest must be valid JSON") + + // Must have "next" key. + next, ok := m[nextKey] + require.True(t, ok, "embedded manifest must have %q key", nextKey) + assert.NotEmpty(t, next.AppKit, "next.appkit must be set") + assert.NotEmpty(t, next.AgentSkills, "next.skills must be set") + + // Must have at least one versioned entry. + var versionedKeys []string + for k := range m { + if k != nextKey { + versionedKeys = append(versionedKeys, k) + } + } + assert.NotEmpty(t, versionedKeys, "must have at least one versioned CLI entry besides %q", nextKey) + + // All versioned keys must be valid semver. + for _, k := range versionedKeys { + assert.True(t, semver.IsValid("v"+k), "key %q must be valid semver", k) + } + + // "next" versions must be >= all versioned entries. + for _, k := range versionedKeys { + entry := m[k] + assert.GreaterOrEqual(t, semver.Compare("v"+next.AppKit, "v"+entry.AppKit), 0, + "next.appkit (%s) must be >= %s.appkit (%s)", next.AppKit, k, entry.AppKit) + assert.GreaterOrEqual(t, semver.Compare("v"+next.AgentSkills, "v"+entry.AgentSkills), 0, + "next.skills (%s) must be >= %s.skills (%s)", next.AgentSkills, k, entry.AgentSkills) + } + + // Versioned keys must be in ascending semver order. + for i := 1; i < len(versionedKeys); i++ { + cmp := semver.Compare("v"+versionedKeys[i-1], "v"+versionedKeys[i]) + assert.LessOrEqual(t, cmp, 0, + "versioned keys must be in ascending order: %s should come before %s", + versionedKeys[i-1], versionedKeys[i]) + } +} + +// --- Local cache helpers --- + +func TestManifestLocalPath(t *testing.T) { + ctx := env.Set(t.Context(), "DATABRICKS_CACHE_DIR", "/tmp/test-cache") + path := manifestLocalPath(ctx) + assert.Equal(t, filepath.Join("/tmp/test-cache", localManifestFile), path) +} + +func TestReadWriteLocalManifest(t *testing.T) { + ctx := testContext(t) + m := Manifest{ + "next": {AppKit: "0.50.0", AgentSkills: "0.5.0"}, + "0.300.0": {AppKit: "0.50.0", AgentSkills: "0.5.0"}, + } + + path := manifestLocalPath(ctx) + writeLocalManifest(ctx, path, m) + + cached, err := readLocalManifest(path) + require.NoError(t, err) + assert.Equal(t, "0.50.0", cached.manifest["next"].AppKit) + assert.True(t, cached.isFresh(cacheTTL), "just-written file should be fresh") +} From 0ae73a26b379e82f7cf6a84d54c8a367fda5ca87 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 15:16:37 +0200 Subject: [PATCH 02/10] fix: address code review findings - Guard printVersionLine against empty latestRef to prevent confusing "Update available: v" output when skills version resolution fails - Sort versionedKeys in TestEmbeddedManifest_IsWellFormed to prevent flaky test from map iteration randomness - Preserve embedded manifest parse error in FetchManifest error message - Add test for GetSkillsRef fallback to embedded manifest Co-authored-by: Isaac --- experimental/aitools/cmd/version.go | 6 ++++++ experimental/aitools/lib/installer/installer_test.go | 12 ++++++++++++ libs/depversions/depversions.go | 5 +++-- libs/depversions/depversions_test.go | 7 +++++++ 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/experimental/aitools/cmd/version.go b/experimental/aitools/cmd/version.go index 17faa468a6..a5bde04e95 100644 --- a/experimental/aitools/cmd/version.go +++ b/experimental/aitools/cmd/version.go @@ -84,6 +84,12 @@ func printVersionLine(ctx context.Context, label string, state *installer.Instal skillNoun = "skill" } + if latestRef == "" { + cmdio.LogString(ctx, fmt.Sprintf(" %s: v%s (%d %s)", label, version, len(state.Skills), skillNoun)) + cmdio.LogString(ctx, " Last updated: "+state.LastUpdated.Format("2006-01-02")) + return + } + if latestRef == state.Release { cmdio.LogString(ctx, fmt.Sprintf(" %s: v%s (%d %s, up to date)", label, version, len(state.Skills), skillNoun)) cmdio.LogString(ctx, " Last updated: "+state.LastUpdated.Format("2006-01-02")) diff --git a/experimental/aitools/lib/installer/installer_test.go b/experimental/aitools/lib/installer/installer_test.go index df3bf8f6b0..36ab7d9dc9 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -742,3 +742,15 @@ func TestSupportsProjectScopeSetCorrectly(t *testing.T) { assert.Equal(t, want, agent.SupportsProjectScope, "SupportsProjectScope for %s", agent.Name) } } + +func TestGetSkillsRefFallsBackToEmbeddedManifest(t *testing.T) { + // Do NOT set DATABRICKS_SKILLS_REF — exercises the production code path + // where the version is resolved from the embedded compatibility manifest. + t.Setenv("DATABRICKS_CACHE_DIR", t.TempDir()) + ctx := t.Context() + + ref, err := GetSkillsRef(ctx) + require.NoError(t, err, "GetSkillsRef should succeed via embedded manifest") + assert.NotEmpty(t, ref) + assert.True(t, len(ref) > 1 && ref[0] == 'v', "ref should start with 'v', got %q", ref) +} diff --git a/libs/depversions/depversions.go b/libs/depversions/depversions.go index da2febc59e..2a08e427a0 100644 --- a/libs/depversions/depversions.go +++ b/libs/depversions/depversions.go @@ -94,12 +94,13 @@ func FetchManifest(ctx context.Context) (Manifest, error) { } // Tier 3b: embedded manifest. - if m, err := parseManifest(build.EmbeddedManifestJSON); err == nil { + m, embeddedErr := parseManifest(build.EmbeddedManifestJSON) + if embeddedErr == nil { log.Debugf(ctx, "Using embedded manifest (remote and local cache failed)") return m, nil } - return nil, fmt.Errorf("all manifest sources failed: %w", fetchErr) + return nil, fmt.Errorf("all manifest sources failed (remote: %w, embedded: %v)", fetchErr, embeddedErr) } // EmbeddedDefaultAppKitVersion returns the "next" entry's AppKit version from diff --git a/libs/depversions/depversions_test.go b/libs/depversions/depversions_test.go index 73be235ad6..876e06ce16 100644 --- a/libs/depversions/depversions_test.go +++ b/libs/depversions/depversions_test.go @@ -12,6 +12,8 @@ import ( "testing" "time" + "slices" + "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/env" "github.com/stretchr/testify/assert" @@ -383,6 +385,11 @@ func TestEmbeddedManifest_IsWellFormed(t *testing.T) { assert.True(t, semver.IsValid("v"+k), "key %q must be valid semver", k) } + // Sort to get deterministic order from map iteration. + slices.SortFunc(versionedKeys, func(a, b string) int { + return semver.Compare("v"+a, "v"+b) + }) + // "next" versions must be >= all versioned entries. for _, k := range versionedKeys { entry := m[k] From 77186ec0c84a3f527779b64b26f9dbdce20924b5 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 15:23:25 +0200 Subject: [PATCH 03/10] Rename depversions to clicompat and improve code quality - Rename libs/depversions/ to libs/clicompat/ (package + files) - Rename internal/build/dep_versions.go to clicompat.go - Rename EmbeddedManifestJSON to CLICompatManifestJSON - Extract devVersionPrefix const for "0.0.0-dev" - Wrap both errors with %w in FetchManifest fallback - Add template-v tag validation to bump-cli-compat skill - Remove confusing positional args from skill (named flags only) - Fix README: "After each AppKit or Agent Skills release" Signed-off-by: Pawel Kosiec --- .agent/skills/bump-cli-compat/SKILL.md | 42 +++++++++++-------- .github/OWNERS | 2 +- cmd/apps/init.go | 6 +-- cmd/apps/manifest.go | 4 +- .../aitools/lib/installer/installer.go | 4 +- internal/build/README.md | 2 +- .../build/{dep_versions.go => clicompat.go} | 4 +- .../depversions.go => clicompat/clicompat.go} | 13 +++--- .../clicompat_test.go} | 6 +-- 9 files changed, 46 insertions(+), 37 deletions(-) rename internal/build/{dep_versions.go => clicompat.go} (55%) rename libs/{depversions/depversions.go => clicompat/clicompat.go} (96%) rename libs/{depversions/depversions_test.go => clicompat/clicompat_test.go} (99%) diff --git a/.agent/skills/bump-cli-compat/SKILL.md b/.agent/skills/bump-cli-compat/SKILL.md index 3256b3f74b..a1fe768776 100644 --- a/.agent/skills/bump-cli-compat/SKILL.md +++ b/.agent/skills/bump-cli-compat/SKILL.md @@ -11,10 +11,10 @@ Updates `internal/build/cli-compat.json` with new AppKit and Agent Skills versio ## Arguments -Parse the user's input for optional version arguments: +Parse the user's input for optional named flags: -- `--appkit ` or first positional arg → AppKit version (e.g. `0.28.0`) -- `--skills ` or second positional arg → Agent Skills version (e.g. `0.1.6`) +- `--appkit ` → AppKit version (e.g. `0.28.0`) +- `--skills ` → Agent Skills version (e.g. `0.1.6`) - No args → auto-detect latest versions from GitHub tags Versions should be provided **without** the `v` prefix (e.g. `0.28.0`, not `v0.28.0`). If provided with the prefix, strip it. @@ -23,9 +23,9 @@ Versions should be provided **without** the `v` prefix (e.g. `0.28.0`, not `v0.2 ### Step 1: Resolve versions -If both `appkit` and `skills` versions were provided as arguments, skip to Step 2. +If both `--appkit` and `--skills` versions were provided, skip to Step 2. -Otherwise, fetch the latest tags from GitHub: +For any missing version, fetch the latest tag from GitHub: ```bash # Latest appkit version (strip leading 'v') @@ -47,14 +47,20 @@ Show the resolved versions to the user and ask: ### Step 2: Validate tags exist -Verify that the corresponding Git tags exist on GitHub: +Verify that the corresponding Git tags exist on GitHub. For AppKit, also validate the `template-v` tag (used by `apps init`): ```bash -gh api repos/databricks/appkit/git/ref/tags/v{appkit_version} --jq '.ref' 2>&1 -gh api repos/databricks/databricks-agent-skills/git/ref/tags/v{skills_version} --jq '.ref' 2>&1 +# AppKit release tag +gh api repos/databricks/appkit/git/ref/tags/v{appkit_version} --jq '.ref' + +# AppKit template tag (used by apps init) +gh api repos/databricks/appkit/git/ref/tags/template-v{appkit_version} --jq '.ref' + +# Agent Skills tag +gh api repos/databricks/databricks-agent-skills/git/ref/tags/v{skills_version} --jq '.ref' ``` -If either tag doesn't exist, report the error and stop. +If any tag doesn't exist, report the error and stop. ### Step 3: Read current manifest @@ -71,7 +77,7 @@ Write the updated `internal/build/cli-compat.json`. Run the Go tests to ensure the manifest is well-formed: ```bash -go test ./libs/depversions/... -run TestEmbeddedManifest -v +go test ./libs/clicompat/... -run TestEmbeddedManifest -v ``` If validation fails, show the errors and fix them before proceeding. @@ -84,12 +90,12 @@ git checkout -b bump-cli-compat-appkit-{appkit_version}-skills-{skills_version} # Stage and commit git add internal/build/cli-compat.json -git commit -s -m "chore: bump cli-compat to appkit {appkit_version}, skills {skills_version}" +git commit -s -m "Bump cli-compat to appkit {appkit_version}, skills {skills_version}" # Push and create PR git push -u origin HEAD gh pr create \ - --title "chore: bump cli-compat to appkit {appkit_version}, skills {skills_version}" \ + --title "Bump cli-compat to appkit {appkit_version}, skills {skills_version}" \ --body "$(cat <<'EOF' ## Summary Bump `cli-compat.json` to use: @@ -98,7 +104,7 @@ Bump `cli-compat.json` to use: ## Checklist - [ ] Evals passed with no regressions -- [ ] `go test ./libs/depversions/... -run TestEmbeddedManifest` passes +- [ ] `go test ./libs/clicompat/... -run TestEmbeddedManifest` passes EOF )" ``` @@ -109,9 +115,9 @@ Show the PR URL to the user when done. ### Example: With explicit versions ``` -/bump-cli-compat 0.28.0 0.1.6 +/bump-cli-compat --appkit 0.28.0 --skills 0.1.6 ``` -Validates tags exist, updates manifest, creates PR. +Validates tags exist (including `template-v0.28.0`), updates manifest, creates PR. ### Example: Auto-detect latest ``` @@ -119,8 +125,8 @@ Validates tags exist, updates manifest, creates PR. ``` Fetches latest tags, asks for eval confirmation, then updates and creates PR. -### Example: With flags +### Example: Only bump AppKit ``` -/bump-cli-compat --appkit 0.28.0 --skills 0.1.6 +/bump-cli-compat --appkit 0.28.0 ``` -Same as positional args. +Auto-detects latest skills version, asks for confirmation, then updates both. diff --git a/.github/OWNERS b/.github/OWNERS index 91084750e8..579fcc2d44 100644 --- a/.github/OWNERS +++ b/.github/OWNERS @@ -61,7 +61,7 @@ # CLI compatibility manifest /internal/build/cli-compat.json team:eng-apps-devex team:platform -/libs/depversions/ team:eng-apps-devex team:platform +/libs/clicompat/ team:eng-apps-devex team:platform # Experimental /experimental/aitools/ team:eng-apps-devex @lennartkats-db diff --git a/cmd/apps/init.go b/cmd/apps/init.go index 5daba0e202..6cd3f5e82c 100644 --- a/cmd/apps/init.go +++ b/cmd/apps/init.go @@ -25,7 +25,7 @@ import ( "github.com/databricks/cli/libs/apps/prompt" "github.com/databricks/cli/libs/cmdctx" "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/depversions" + "github.com/databricks/cli/libs/clicompat" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/log" @@ -170,7 +170,7 @@ Environment variables: cmd.Flags().StringVar(&templatePath, "template", "", "Template path (local directory or GitHub URL)") cmd.Flags().StringVar(&branch, "branch", "", "Git branch or tag (for GitHub templates, mutually exclusive with --version)") versionDesc := "AppKit version to use (use 'latest' for main branch)" - if v := depversions.EmbeddedDefaultAppKitVersion(); v != "" { + if v := clicompat.EmbeddedDefaultAppKitVersion(); v != "" { versionDesc = fmt.Sprintf("AppKit version to use (default: %s, use 'latest' for main branch)", v) } cmd.Flags().StringVar(&version, "version", "", versionDesc) @@ -809,7 +809,7 @@ func runCreate(ctx context.Context, opts createOptions) error { case opts.version != "": gitRef = normalizeVersion(opts.version) default: - appkitVersion, err := depversions.ResolveAppKitVersion(ctx) + appkitVersion, err := clicompat.ResolveAppKitVersion(ctx) if err != nil { return fmt.Errorf("could not resolve AppKit template version: %w. Use --version to specify a version manually", err) } diff --git a/cmd/apps/manifest.go b/cmd/apps/manifest.go index ff200e6998..96b000aa18 100644 --- a/cmd/apps/manifest.go +++ b/cmd/apps/manifest.go @@ -9,7 +9,7 @@ import ( "path/filepath" "github.com/databricks/cli/libs/apps/manifest" - "github.com/databricks/cli/libs/depversions" + "github.com/databricks/cli/libs/clicompat" "github.com/databricks/cli/libs/env" "github.com/spf13/cobra" ) @@ -28,7 +28,7 @@ func runManifestOnly(ctx context.Context, templatePath, branch, version string) case version != "": gitRef = normalizeVersion(version) default: - appkitVersion, err := depversions.ResolveAppKitVersion(ctx) + appkitVersion, err := clicompat.ResolveAppKitVersion(ctx) if err != nil { return fmt.Errorf("could not resolve AppKit template version: %w. Use --version to specify a version manually", err) } diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 39e04120ba..60c7e16080 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -17,7 +17,7 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/depversions" + "github.com/databricks/cli/libs/clicompat" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" "golang.org/x/mod/semver" @@ -39,7 +39,7 @@ func GetSkillsRef(ctx context.Context) (string, error) { if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" { return ref, nil } - v, err := depversions.ResolveAgentSkillsVersion(ctx) + v, err := clicompat.ResolveAgentSkillsVersion(ctx) if err != nil { return "", fmt.Errorf("could not resolve skills version: %w", err) } diff --git a/internal/build/README.md b/internal/build/README.md index 71409737c7..d162fe2b3f 100644 --- a/internal/build/README.md +++ b/internal/build/README.md @@ -34,7 +34,7 @@ At runtime, the CLI resolves the manifest from three sources: ## When to update -After each AppKit release: +After each AppKit or Agent Skills release: 1. **Run evals** on the new AppKit version. If there is no regression, proceed. 2. **Open a PR** to update `cli-compat.json`. The change depends on the type of release: diff --git a/internal/build/dep_versions.go b/internal/build/clicompat.go similarity index 55% rename from internal/build/dep_versions.go rename to internal/build/clicompat.go index c5bc84635c..22cb8f13a0 100644 --- a/internal/build/dep_versions.go +++ b/internal/build/clicompat.go @@ -2,8 +2,8 @@ package build import _ "embed" -// EmbeddedManifestJSON is the cli-compat.json manifest embedded at compile time. +// CLICompatManifestJSON is the cli-compat.json manifest embedded at compile time. // Used as the last-resort fallback when both remote fetch and local cache fail. // //go:embed cli-compat.json -var EmbeddedManifestJSON []byte +var CLICompatManifestJSON []byte diff --git a/libs/depversions/depversions.go b/libs/clicompat/clicompat.go similarity index 96% rename from libs/depversions/depversions.go rename to libs/clicompat/clicompat.go index 2a08e427a0..5f7941f871 100644 --- a/libs/depversions/depversions.go +++ b/libs/clicompat/clicompat.go @@ -1,4 +1,4 @@ -package depversions +package clicompat import ( "context" @@ -35,6 +35,9 @@ const ( // localManifestFile is the filename for the locally cached manifest. localManifestFile = "compat-manifest.json" + // devVersionPrefix identifies dev builds that always use the "next" entry. + devVersionPrefix = "0.0.0-dev" + fetchRetries = 2 fetchRetryBackoff = 300 * time.Millisecond ) @@ -94,20 +97,20 @@ func FetchManifest(ctx context.Context) (Manifest, error) { } // Tier 3b: embedded manifest. - m, embeddedErr := parseManifest(build.EmbeddedManifestJSON) + m, embeddedErr := parseManifest(build.CLICompatManifestJSON) if embeddedErr == nil { log.Debugf(ctx, "Using embedded manifest (remote and local cache failed)") return m, nil } - return nil, fmt.Errorf("all manifest sources failed (remote: %w, embedded: %v)", fetchErr, embeddedErr) + return nil, fmt.Errorf("all manifest sources failed (remote: %w, embedded: %w)", fetchErr, embeddedErr) } // EmbeddedDefaultAppKitVersion returns the "next" entry's AppKit version from // the embedded manifest. Used for help text defaults where a network call is // not appropriate. Returns "" if the embedded manifest is invalid. func EmbeddedDefaultAppKitVersion() string { - m, err := parseManifest(build.EmbeddedManifestJSON) + m, err := parseManifest(build.CLICompatManifestJSON) if err != nil { return "" } @@ -136,7 +139,7 @@ func Resolve(m Manifest, cliVersion string) (Entry, error) { } // Dev builds always use "next". - if strings.HasPrefix(cliVersion, "0.0.0-dev") { + if strings.HasPrefix(cliVersion, devVersionPrefix) { return next, nil } diff --git a/libs/depversions/depversions_test.go b/libs/clicompat/clicompat_test.go similarity index 99% rename from libs/depversions/depversions_test.go rename to libs/clicompat/clicompat_test.go index 876e06ce16..73459e3290 100644 --- a/libs/depversions/depversions_test.go +++ b/libs/clicompat/clicompat_test.go @@ -1,4 +1,4 @@ -package depversions +package clicompat import ( "context" @@ -178,7 +178,7 @@ func TestFetchManifest_RemoteFailFallsBackToEmbedded(t *testing.T) { require.NoError(t, err) // Verify it returned the embedded manifest values. - embedded, _ := parseManifest(build.EmbeddedManifestJSON) + embedded, _ := parseManifest(build.CLICompatManifestJSON) assert.Equal(t, embedded["next"].AppKit, result["next"].AppKit) } @@ -362,7 +362,7 @@ func TestEmbeddedDefaultAppKitVersion(t *testing.T) { // --- Embedded manifest validation (replaces AppKit TS validator) --- func TestEmbeddedManifest_IsWellFormed(t *testing.T) { - m, err := parseManifest(build.EmbeddedManifestJSON) + m, err := parseManifest(build.CLICompatManifestJSON) require.NoError(t, err, "embedded manifest must be valid JSON") // Must have "next" key. From 41ecb8a73b1123607499240dd4ba4f044c41fa26 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 15:29:51 +0200 Subject: [PATCH 04/10] Update cli-compat manifest to CLI 0.300.0 and skills 0.1.5 Signed-off-by: Pawel Kosiec --- internal/build/cli-compat.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/build/cli-compat.json b/internal/build/cli-compat.json index b7cff3b831..33fc41f0a6 100644 --- a/internal/build/cli-compat.json +++ b/internal/build/cli-compat.json @@ -1,4 +1,4 @@ { - "next": { "appkit": "0.24.0", "skills": "0.1.4" }, - "0.299.0": { "appkit": "0.24.0", "skills": "0.1.4" } + "next": { "appkit": "0.24.0", "skills": "0.1.5" }, + "0.300.0": { "appkit": "0.24.0", "skills": "0.1.5" } } From 4e8a2054d4323137714a891d48159d9645853adc Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 15:58:16 +0200 Subject: [PATCH 05/10] fix: address code review findings - Fix stale libs/depversions/ references in README (renamed to libs/clicompat/) - Fix incorrect "next" key description (only used for dev builds, not newer-than-all) - Fix import ordering (clicompat before cmdctx/cmdio alphabetically) - Fix slices import grouping in clicompat_test.go (stdlib, not separate group) - Fix error format: semicolon instead of period before hint text - Fix FetchManifest godoc: "4-tier fallback" to match numbered list - Fix writeLocalManifest comment: explain temp-file-then-rename pattern - Fix README example values to match actual manifest - Fix flaky test: pre-populate cache to avoid real network calls Co-authored-by: Isaac --- cmd/apps/init.go | 4 ++-- cmd/apps/manifest.go | 2 +- .../aitools/lib/installer/installer.go | 2 +- .../aitools/lib/installer/installer_test.go | 20 +++++++++++-------- internal/build/README.md | 12 +++++------ libs/clicompat/clicompat.go | 6 ++++-- libs/clicompat/clicompat_test.go | 3 +-- 7 files changed, 27 insertions(+), 22 deletions(-) diff --git a/cmd/apps/init.go b/cmd/apps/init.go index 6cd3f5e82c..01bfa0fad6 100644 --- a/cmd/apps/init.go +++ b/cmd/apps/init.go @@ -23,9 +23,9 @@ import ( "github.com/databricks/cli/libs/apps/initializer" "github.com/databricks/cli/libs/apps/manifest" "github.com/databricks/cli/libs/apps/prompt" + "github.com/databricks/cli/libs/clicompat" "github.com/databricks/cli/libs/cmdctx" "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/clicompat" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/log" @@ -811,7 +811,7 @@ func runCreate(ctx context.Context, opts createOptions) error { default: appkitVersion, err := clicompat.ResolveAppKitVersion(ctx) if err != nil { - return fmt.Errorf("could not resolve AppKit template version: %w. Use --version to specify a version manually", err) + return fmt.Errorf("could not resolve AppKit template version: %w; use --version to specify a version manually", err) } gitRef = normalizeVersion(appkitVersion) cmdio.LogString(ctx, "Using AppKit template version "+appkitVersion) diff --git a/cmd/apps/manifest.go b/cmd/apps/manifest.go index 96b000aa18..da904b1471 100644 --- a/cmd/apps/manifest.go +++ b/cmd/apps/manifest.go @@ -30,7 +30,7 @@ func runManifestOnly(ctx context.Context, templatePath, branch, version string) default: appkitVersion, err := clicompat.ResolveAppKitVersion(ctx) if err != nil { - return fmt.Errorf("could not resolve AppKit template version: %w. Use --version to specify a version manually", err) + return fmt.Errorf("could not resolve AppKit template version: %w; use --version to specify a version manually", err) } gitRef = normalizeVersion(appkitVersion) } diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 60c7e16080..c8e3c7698b 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -16,8 +16,8 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/internal/build" - "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/clicompat" + "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" "golang.org/x/mod/semver" diff --git a/experimental/aitools/lib/installer/installer_test.go b/experimental/aitools/lib/installer/installer_test.go index 36ab7d9dc9..848ab54172 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -743,14 +743,18 @@ func TestSupportsProjectScopeSetCorrectly(t *testing.T) { } } -func TestGetSkillsRefFallsBackToEmbeddedManifest(t *testing.T) { - // Do NOT set DATABRICKS_SKILLS_REF — exercises the production code path - // where the version is resolved from the embedded compatibility manifest. - t.Setenv("DATABRICKS_CACHE_DIR", t.TempDir()) - ctx := t.Context() - - ref, err := GetSkillsRef(ctx) - require.NoError(t, err, "GetSkillsRef should succeed via embedded manifest") +func TestGetSkillsRefResolvesFromManifest(t *testing.T) { + // Pre-populate the cache so FetchManifest returns from tier 1 (local cache) + // without hitting the network. The embedded manifest fallback is tested + // separately in clicompat_test.go. + cacheDir := t.TempDir() + t.Setenv("DATABRICKS_CACHE_DIR", cacheDir) + cachePath := filepath.Join(cacheDir, "compat-manifest.json") + manifest := `{"next":{"appkit":"0.24.0","skills":"0.1.5"},"0.300.0":{"appkit":"0.24.0","skills":"0.1.5"}}` + require.NoError(t, os.WriteFile(cachePath, []byte(manifest), 0o644)) + + ref, err := GetSkillsRef(t.Context()) + require.NoError(t, err, "GetSkillsRef should succeed via cached manifest") assert.NotEmpty(t, ref) assert.True(t, len(ref) > 1 && ref[0] == 'v', "ref should start with 'v', got %q", ref) } diff --git a/internal/build/README.md b/internal/build/README.md index d162fe2b3f..48ddcad152 100644 --- a/internal/build/README.md +++ b/internal/build/README.md @@ -6,14 +6,14 @@ ```json { - "next": { "appkit": "0.24.0", "skills": "0.1.4" }, - "0.299.0": { "appkit": "0.24.0", "skills": "0.1.4" } + "next": { "appkit": "0.24.0", "skills": "0.1.5" }, + "0.300.0": { "appkit": "0.24.0", "skills": "0.1.5" } } ``` - Each key is a CLI version (`X.Y.Z`) or `"next"`. - Each value specifies the compatible `appkit` and `skills` versions. -- `"next"` is used for CLI versions newer than any listed entry and for dev builds. +- `"next"` is used for dev builds (`0.0.0-dev*`). For production CLI versions newer than all listed entries, the highest versioned entry is used. ## How the CLI resolves versions @@ -25,7 +25,7 @@ ## Manifest sources (fallback chain) -At runtime, the CLI resolves the manifest from three sources: +At runtime, the CLI resolves the manifest from four sources: 1. **Local cache** (`~/.cache/databricks/compat-manifest.json`) — used if fresh (< 1 hour old). 2. **Remote fetch** from GitHub — used when cache is stale or missing. On success, the local cache is updated. @@ -46,10 +46,10 @@ This process is manual for now but can be automated as part of the release workf ## Validation -The manifest is validated by Go tests in `libs/depversions/`: +The manifest is validated by Go tests in `libs/clicompat/`: ```bash -go test ./libs/depversions/... -run TestEmbeddedManifest -v +go test ./libs/clicompat/... -run TestEmbeddedManifest -v ``` This checks: valid JSON, `"next"` key present, at least one versioned entry, valid semver keys, `next` versions >= all entries, and ascending key order. diff --git a/libs/clicompat/clicompat.go b/libs/clicompat/clicompat.go index 5f7941f871..a9b476a833 100644 --- a/libs/clicompat/clicompat.go +++ b/libs/clicompat/clicompat.go @@ -66,7 +66,7 @@ func (c cachedManifest) isFresh(maxAge time.Duration) bool { // so tests can replace it. var httpClient = &http.Client{Timeout: fetchTimeout} -// FetchManifest returns the compatibility manifest using a 3-tier strategy: +// FetchManifest returns the compatibility manifest using a 4-tier fallback: // 1. Local cached file (if fresh, < 1 hour old) // 2. Remote fetch from GitHub (with retry) // 3. Stale local file (if remote fails but a previously cached file exists) @@ -238,7 +238,9 @@ func readLocalManifest(path string) (cachedManifest, error) { return cachedManifest{manifest: m, modTime: info.ModTime()}, nil } -// writeLocalManifest atomically writes the manifest to the local cache path. +// writeLocalManifest writes the manifest to the local cache path using a +// temp-file-then-rename pattern. The os.Remove before os.Rename is needed +// for Windows, where Rename fails if the destination exists. func writeLocalManifest(ctx context.Context, path string, m Manifest) { if path == "" { return diff --git a/libs/clicompat/clicompat_test.go b/libs/clicompat/clicompat_test.go index 73459e3290..7fc9aeabc6 100644 --- a/libs/clicompat/clicompat_test.go +++ b/libs/clicompat/clicompat_test.go @@ -8,12 +8,11 @@ import ( "net/url" "os" "path/filepath" + "slices" "sync/atomic" "testing" "time" - "slices" - "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/env" "github.com/stretchr/testify/assert" From 0262d6470af7503cb110c6b9df2cee8c8c31f453 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Wed, 6 May 2026 10:03:01 +0200 Subject: [PATCH 06/10] Fall back to embedded manifest when resolved version is not found When the manifest resolves to a version that doesn't exist as a git tag (404), retry with the version from the embedded manifest. Only triggers on "not found" errors, not transient network failures. Also: - Rename EmbeddedDefaultAppKitVersion/EmbeddedResolve* to ResolveEmbeddedAppKitVersion/ResolveEmbeddedAgentSkillsVersion - Remove duplicate log lines (keep only log.Warnf, drop cmdio.LogString) - Drop ctx parameter from ResolveEmbedded* (not needed) Signed-off-by: Pawel Kosiec --- cmd/apps/init.go | 13 +++++- .../aitools/lib/installer/installer.go | 10 +++++ libs/clicompat/clicompat.go | 44 +++++++++++++++---- libs/clicompat/clicompat_test.go | 11 ++--- 4 files changed, 64 insertions(+), 14 deletions(-) diff --git a/cmd/apps/init.go b/cmd/apps/init.go index 01bfa0fad6..35eb45cd99 100644 --- a/cmd/apps/init.go +++ b/cmd/apps/init.go @@ -170,7 +170,7 @@ Environment variables: cmd.Flags().StringVar(&templatePath, "template", "", "Template path (local directory or GitHub URL)") cmd.Flags().StringVar(&branch, "branch", "", "Git branch or tag (for GitHub templates, mutually exclusive with --version)") versionDesc := "AppKit version to use (use 'latest' for main branch)" - if v := clicompat.EmbeddedDefaultAppKitVersion(); v != "" { + if v, err := clicompat.ResolveEmbeddedAppKitVersion(); err == nil && v != "" { versionDesc = fmt.Sprintf("AppKit version to use (default: %s, use 'latest' for main branch)", v) } cmd.Flags().StringVar(&version, "version", "", versionDesc) @@ -867,6 +867,17 @@ func runCreate(ctx context.Context, opts createOptions) error { // Step 2: Wait for template (may already be done if the user took time typing the name) resolvedPath, cleanup, err := awaitTemplate(ctx, templateCh) + if err != nil && usingDefaultTemplate && clicompat.IsNotFoundError(err) { + // The resolved version doesn't exist as a tag. Fall back to the + // embedded manifest which ships a known-good version. + fallbackVersion, fbErr := clicompat.ResolveEmbeddedAppKitVersion() + if fbErr == nil && fallbackVersion != "" { + log.Warnf(ctx, "Template version not found, falling back to embedded version %s", fallbackVersion) + fallbackRef := normalizeVersion(fallbackVersion) + templateCh = resolveTemplateAsync(ctx, templateSrc, fallbackRef, appkitTemplateDir) + resolvedPath, cleanup, err = awaitTemplate(ctx, templateCh) + } + } if err != nil { return err } diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index c8e3c7698b..1f081f69fc 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -104,6 +104,16 @@ func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgent tag := strings.TrimPrefix(ref, "v") cmdio.LogString(ctx, "Using skills version "+tag) manifest, err := src.FetchManifest(ctx, ref) + if err != nil && clicompat.IsNotFoundError(err) { + // The resolved version doesn't exist. Fall back to the embedded manifest. + fallbackVersion, fbErr := clicompat.ResolveEmbeddedAgentSkillsVersion() + if fbErr == nil && fallbackVersion != "" && fallbackVersion != tag { + log.Warnf(ctx, "Skills version %s not found, falling back to embedded version %s", tag, fallbackVersion) + ref = "v" + fallbackVersion + tag = fallbackVersion + manifest, err = src.FetchManifest(ctx, ref) + } + } if err != nil { return err } diff --git a/libs/clicompat/clicompat.go b/libs/clicompat/clicompat.go index a9b476a833..e77ec7da9c 100644 --- a/libs/clicompat/clicompat.go +++ b/libs/clicompat/clicompat.go @@ -106,18 +106,46 @@ func FetchManifest(ctx context.Context) (Manifest, error) { return nil, fmt.Errorf("all manifest sources failed (remote: %w, embedded: %w)", fetchErr, embeddedErr) } -// EmbeddedDefaultAppKitVersion returns the "next" entry's AppKit version from -// the embedded manifest. Used for help text defaults where a network call is -// not appropriate. Returns "" if the embedded manifest is invalid. -func EmbeddedDefaultAppKitVersion() string { +// ResolveEmbeddedAppKitVersion resolves the AppKit version from only the +// embedded manifest for the current CLI version. Used as a fallback when the +// primary version (from remote or cached manifest) points to a non-existent tag, +// and for help text defaults where a network call is not appropriate. +func ResolveEmbeddedAppKitVersion() (string, error) { m, err := parseManifest(build.CLICompatManifestJSON) if err != nil { - return "" + return "", fmt.Errorf("embedded manifest: %w", err) + } + entry, err := Resolve(m, build.GetInfo().Version) + if err != nil { + return "", fmt.Errorf("embedded manifest resolve: %w", err) + } + return entry.AppKit, nil +} + +// ResolveEmbeddedAgentSkillsVersion resolves the Agent Skills version from only +// the embedded manifest for the current CLI version. Used as a fallback when the +// primary version points to a non-existent tag. +func ResolveEmbeddedAgentSkillsVersion() (string, error) { + m, err := parseManifest(build.CLICompatManifestJSON) + if err != nil { + return "", fmt.Errorf("embedded manifest: %w", err) + } + entry, err := Resolve(m, build.GetInfo().Version) + if err != nil { + return "", fmt.Errorf("embedded manifest resolve: %w", err) } - if next, ok := m[nextKey]; ok { - return next.AppKit + return entry.AgentSkills, nil +} + +// IsNotFoundError reports whether the error indicates a "not found" condition +// (e.g. HTTP 404, missing git branch/tag). Used by consumers to decide whether +// to fall back to the embedded manifest. +func IsNotFoundError(err error) bool { + if err == nil { + return false } - return "" + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "not found") || strings.Contains(msg, "404") } // Resolve returns the manifest entry for the given CLI version. diff --git a/libs/clicompat/clicompat_test.go b/libs/clicompat/clicompat_test.go index 7fc9aeabc6..f40f64dd80 100644 --- a/libs/clicompat/clicompat_test.go +++ b/libs/clicompat/clicompat_test.go @@ -350,12 +350,13 @@ func TestResolveEntry_RemoteFailUsesEmbedded(t *testing.T) { assert.NotEmpty(t, entry.AppKit) } -// --- EmbeddedDefaultAppKitVersion --- +// --- ResolveEmbeddedAppKitVersion --- -func TestEmbeddedDefaultAppKitVersion(t *testing.T) { - v := EmbeddedDefaultAppKitVersion() - assert.NotEmpty(t, v, "embedded manifest should have a next.appkit version") - assert.True(t, semver.IsValid("v"+v), "embedded default version should be valid semver") +func TestResolveEmbeddedAppKitVersion(t *testing.T) { + v, err := ResolveEmbeddedAppKitVersion() + require.NoError(t, err) + assert.NotEmpty(t, v, "embedded manifest should resolve an appkit version") + assert.True(t, semver.IsValid("v"+v), "embedded resolved version should be valid semver") } // --- Embedded manifest validation (replaces AppKit TS validator) --- From 174446890efd78bade35380224e7b5d6560640b6 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Wed, 6 May 2026 15:38:24 +0200 Subject: [PATCH 07/10] fix: address code review findings for clicompat Address all review comments: add sentinel ErrNotFound and typed HTTPStatusError, validate manifest entry values, centralize not-found fallback for skills and AppKit consumers, skip retries on 4xx, add User-Agent header, support DATABRICKS_CACHE_ENABLED, and document pruning policy and trust model. Co-authored-by: Isaac --- cmd/apps/init.go | 6 +- cmd/apps/manifest.go | 9 ++ experimental/aitools/cmd/list.go | 2 +- .../aitools/lib/installer/installer.go | 29 +++-- experimental/aitools/lib/installer/source.go | 4 + experimental/aitools/lib/installer/update.go | 2 +- internal/build/README.md | 10 +- libs/clicompat/clicompat.go | 104 +++++++++++++--- libs/clicompat/clicompat_test.go | 117 ++++++++++++++++++ 9 files changed, 246 insertions(+), 37 deletions(-) diff --git a/cmd/apps/init.go b/cmd/apps/init.go index 35eb45cd99..d7844cce10 100644 --- a/cmd/apps/init.go +++ b/cmd/apps/init.go @@ -169,11 +169,7 @@ Environment variables: cmd.Flags().StringVar(&templatePath, "template", "", "Template path (local directory or GitHub URL)") cmd.Flags().StringVar(&branch, "branch", "", "Git branch or tag (for GitHub templates, mutually exclusive with --version)") - versionDesc := "AppKit version to use (use 'latest' for main branch)" - if v, err := clicompat.ResolveEmbeddedAppKitVersion(); err == nil && v != "" { - versionDesc = fmt.Sprintf("AppKit version to use (default: %s, use 'latest' for main branch)", v) - } - cmd.Flags().StringVar(&version, "version", "", versionDesc) + cmd.Flags().StringVar(&version, "version", "", "AppKit version to use (default: auto-detected, use 'latest' for main branch)") cmd.Flags().StringVar(&name, "name", "", "Project name (prompts if not provided)") cmd.Flags().StringVar(&warehouseID, "warehouse-id", "", "SQL warehouse ID") _ = cmd.Flags().MarkDeprecated("warehouse-id", "use --set .sql-warehouse.id= instead") diff --git a/cmd/apps/manifest.go b/cmd/apps/manifest.go index da904b1471..b79e04c6ff 100644 --- a/cmd/apps/manifest.go +++ b/cmd/apps/manifest.go @@ -11,6 +11,7 @@ import ( "github.com/databricks/cli/libs/apps/manifest" "github.com/databricks/cli/libs/clicompat" "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/log" "github.com/spf13/cobra" ) @@ -44,6 +45,14 @@ func runManifestOnly(ctx context.Context, templatePath, branch, version string) subdirForClone = appkitTemplateDir } resolvedPath, cleanup, err := resolveTemplate(ctx, templateSrc, branchForClone, subdirForClone) + if err != nil && usingDefaultTemplate && clicompat.IsNotFoundError(err) { + fallbackVersion, fbErr := clicompat.ResolveEmbeddedAppKitVersion() + if fbErr == nil && fallbackVersion != "" { + log.Warnf(ctx, "Template version not found, falling back to embedded version %s", fallbackVersion) + fallbackRef := normalizeVersion(fallbackVersion) + resolvedPath, cleanup, err = resolveTemplate(ctx, templateSrc, fallbackRef, appkitTemplateDir) + } + } if err != nil { return err } diff --git a/experimental/aitools/cmd/list.go b/experimental/aitools/cmd/list.go index 8f665d7ad1..56d323a142 100644 --- a/experimental/aitools/cmd/list.go +++ b/experimental/aitools/cmd/list.go @@ -54,7 +54,7 @@ func defaultListSkills(cmd *cobra.Command, scope string) error { } src := &installer.GitHubManifestSource{} - manifest, err := src.FetchManifest(ctx, ref) + manifest, ref, err := installer.FetchSkillsManifestWithFallback(ctx, src, ref) if err != nil { return fmt.Errorf("failed to fetch manifest: %w", err) } diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 1f081f69fc..5bdeb871bb 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -93,27 +93,34 @@ func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byt return io.ReadAll(resp.Body) } -// InstallSkillsForAgents fetches the manifest and installs skills for the given agents. -// This is the core installation function. Callers are responsible for agent detection, -// prompting, and printing the "Installing..." header. -func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgents []*agents.Agent, opts InstallOptions) error { - ref, err := GetSkillsRef(ctx) - if err != nil { - return err - } +// FetchSkillsManifestWithFallback fetches the skills manifest at the given ref. +// If the ref points to a non-existent tag (not-found error), it falls back to +// the embedded manifest's skills version. Returns the manifest, the (possibly +// updated) ref, and any error. +func FetchSkillsManifestWithFallback(ctx context.Context, src ManifestSource, ref string) (*Manifest, string, error) { tag := strings.TrimPrefix(ref, "v") - cmdio.LogString(ctx, "Using skills version "+tag) manifest, err := src.FetchManifest(ctx, ref) if err != nil && clicompat.IsNotFoundError(err) { - // The resolved version doesn't exist. Fall back to the embedded manifest. fallbackVersion, fbErr := clicompat.ResolveEmbeddedAgentSkillsVersion() if fbErr == nil && fallbackVersion != "" && fallbackVersion != tag { log.Warnf(ctx, "Skills version %s not found, falling back to embedded version %s", tag, fallbackVersion) ref = "v" + fallbackVersion - tag = fallbackVersion manifest, err = src.FetchManifest(ctx, ref) } } + return manifest, ref, err +} + +// InstallSkillsForAgents fetches the manifest and installs skills for the given agents. +// This is the core installation function. Callers are responsible for agent detection, +// prompting, and printing the "Installing..." header. +func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgents []*agents.Agent, opts InstallOptions) error { + ref, err := GetSkillsRef(ctx) + if err != nil { + return err + } + cmdio.LogString(ctx, "Using skills version "+strings.TrimPrefix(ref, "v")) + manifest, ref, err := FetchSkillsManifestWithFallback(ctx, src, ref) if err != nil { return err } diff --git a/experimental/aitools/lib/installer/source.go b/experimental/aitools/lib/installer/source.go index e601b26d66..5dc78a254f 100644 --- a/experimental/aitools/lib/installer/source.go +++ b/experimental/aitools/lib/installer/source.go @@ -7,6 +7,7 @@ import ( "net/http" "time" + "github.com/databricks/cli/libs/clicompat" "github.com/databricks/cli/libs/log" ) @@ -37,6 +38,9 @@ func (s *GitHubManifestSource) FetchManifest(ctx context.Context, ref string) (* } defer resp.Body.Close() + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("skills manifest at %s@%s: %w", skillsRepoName, ref, clicompat.ErrNotFound) + } if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("failed to fetch manifest: HTTP %d", resp.StatusCode) } diff --git a/experimental/aitools/lib/installer/update.go b/experimental/aitools/lib/installer/update.go index 0aa260f115..ea3674116b 100644 --- a/experimental/aitools/lib/installer/update.go +++ b/experimental/aitools/lib/installer/update.go @@ -91,7 +91,7 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent return &UpdateResult{Unchanged: slices.Sorted(maps.Keys(state.Skills))}, nil } - manifest, err := src.FetchManifest(ctx, latestTag) + manifest, latestTag, err := FetchSkillsManifestWithFallback(ctx, src, latestTag) if err != nil { if opts.Check { log.Warnf(ctx, "Could not fetch manifest: %v", err) diff --git a/internal/build/README.md b/internal/build/README.md index 48ddcad152..82e3b5df9d 100644 --- a/internal/build/README.md +++ b/internal/build/README.md @@ -52,4 +52,12 @@ The manifest is validated by Go tests in `libs/clicompat/`: go test ./libs/clicompat/... -run TestEmbeddedManifest -v ``` -This checks: valid JSON, `"next"` key present, at least one versioned entry, valid semver keys, `next` versions >= all entries, and ascending key order. +This checks: valid JSON, `"next"` key present, at least one versioned entry, valid semver keys, valid semver entry values, `next` versions >= all entries, and ascending key order. + +## Pruning policy + +Entries MUST NOT be removed from the manifest. Older CLI binaries use the lowest entry as their floor when the CLI version is older than all entries. Pruning it causes them to silently resolve to a newer entry that may require CLI features they lack. If the manifest grows too large, consider archiving very old entries to a separate file while keeping the oldest entry as a sentinel. + +## Trust model + +The live manifest is fetched over HTTPS from GitHub (`raw.githubusercontent.com`). The trust boundary is: TLS certificate validation + GitHub's access controls + write access to the `main` branch of `databricks/cli`. A compromised manifest can only steer clients to existing published tags (AppKit or skills); it cannot inject arbitrary code. The CLI binary always ships an embedded fallback manifest that limits exposure to cache-TTL windows (1 hour). The local cache (`~/.cache/databricks/compat-manifest.json`) is trust-on-disk: an attacker with user-level write access to the cache directory could swap in a malicious manifest pointing to different tags. diff --git a/libs/clicompat/clicompat.go b/libs/clicompat/clicompat.go index e77ec7da9c..0ffe76e482 100644 --- a/libs/clicompat/clicompat.go +++ b/libs/clicompat/clicompat.go @@ -11,6 +11,7 @@ import ( "path/filepath" "slices" "strings" + "sync" "time" "github.com/databricks/cli/internal/build" @@ -19,6 +20,20 @@ import ( "golang.org/x/mod/semver" ) +// ErrNotFound indicates that a requested resource (tag, branch, manifest) +// does not exist at the remote. +var ErrNotFound = errors.New("not found") + +// HTTPStatusError captures a non-200 HTTP status code from a manifest fetch. +type HTTPStatusError struct { + StatusCode int +} + +// Error implements the error interface. +func (e *HTTPStatusError) Error() string { + return fmt.Sprintf("HTTP %d fetching manifest", e.StatusCode) +} + const ( // manifestURL is the raw GitHub URL for the compatibility manifest. manifestURL = "https://raw.githubusercontent.com/databricks/cli/main/internal/build/cli-compat.json" @@ -38,7 +53,7 @@ const ( // devVersionPrefix identifies dev builds that always use the "next" entry. devVersionPrefix = "0.0.0-dev" - fetchRetries = 2 + maxFetchAttempts = 3 fetchRetryBackoff = 300 * time.Millisecond ) @@ -63,7 +78,8 @@ func (c cachedManifest) isFresh(maxAge time.Duration) bool { } // httpClient is the HTTP client used for manifest fetches. Package-level var -// so tests can replace it. +// so tests can replace it. Not safe for parallel tests; the clicompat test +// suite does not use t.Parallel(). var httpClient = &http.Client{Timeout: fetchTimeout} // FetchManifest returns the compatibility manifest using a 4-tier fallback: @@ -71,11 +87,25 @@ var httpClient = &http.Client{Timeout: fetchTimeout} // 2. Remote fetch from GitHub (with retry) // 3. Stale local file (if remote fails but a previously cached file exists) // 4. Embedded manifest compiled into the binary +// +// Set DATABRICKS_CACHE_ENABLED=false to bypass the local cache (tiers 1 and 3a), +// which is useful to recover from a bad cached manifest. func FetchManifest(ctx context.Context) (Manifest, error) { + cacheEnabled := true + if enabled, ok := env.GetBool(ctx, "DATABRICKS_CACHE_ENABLED"); ok { + cacheEnabled = enabled + } + localPath := manifestLocalPath(ctx) // Read local file once — reuse across tiers. - local, localErr := readLocalManifest(localPath) + var local cachedManifest + var localErr error + if cacheEnabled { + local, localErr = readLocalManifest(localPath) + } else { + localErr = errors.New("cache disabled") + } // Tier 1: local file is fresh. if localErr == nil && local.isFresh(cacheTTL) { @@ -86,7 +116,9 @@ func FetchManifest(ctx context.Context) (Manifest, error) { // Tier 2: fetch from remote (local file missing or stale). m, fetchErr := fetchRemoteWithRetry(ctx) if fetchErr == nil { - writeLocalManifest(ctx, localPath, m) + if cacheEnabled { + writeLocalManifest(ctx, localPath, m) + } return m, nil } @@ -97,7 +129,7 @@ func FetchManifest(ctx context.Context) (Manifest, error) { } // Tier 3b: embedded manifest. - m, embeddedErr := parseManifest(build.CLICompatManifestJSON) + m, embeddedErr := parseEmbeddedManifest() if embeddedErr == nil { log.Debugf(ctx, "Using embedded manifest (remote and local cache failed)") return m, nil @@ -106,12 +138,17 @@ func FetchManifest(ctx context.Context) (Manifest, error) { return nil, fmt.Errorf("all manifest sources failed (remote: %w, embedded: %w)", fetchErr, embeddedErr) } +// parseEmbeddedManifest parses the embedded manifest once and caches the result. +var parseEmbeddedManifest = sync.OnceValues(func() (Manifest, error) { + return parseManifest(build.CLICompatManifestJSON) +}) + // ResolveEmbeddedAppKitVersion resolves the AppKit version from only the // embedded manifest for the current CLI version. Used as a fallback when the // primary version (from remote or cached manifest) points to a non-existent tag, // and for help text defaults where a network call is not appropriate. func ResolveEmbeddedAppKitVersion() (string, error) { - m, err := parseManifest(build.CLICompatManifestJSON) + m, err := parseEmbeddedManifest() if err != nil { return "", fmt.Errorf("embedded manifest: %w", err) } @@ -126,7 +163,7 @@ func ResolveEmbeddedAppKitVersion() (string, error) { // the embedded manifest for the current CLI version. Used as a fallback when the // primary version points to a non-existent tag. func ResolveEmbeddedAgentSkillsVersion() (string, error) { - m, err := parseManifest(build.CLICompatManifestJSON) + m, err := parseEmbeddedManifest() if err != nil { return "", fmt.Errorf("embedded manifest: %w", err) } @@ -144,8 +181,18 @@ func IsNotFoundError(err error) bool { if err == nil { return false } + if errors.Is(err, ErrNotFound) { + return true + } + var httpErr *HTTPStatusError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return true + } + // Git clone errors include "not found" in stderr when a branch/tag does not + // exist (e.g. "Remote branch X not found in upstream origin"). This is a + // pragmatic fallback until git.Clone wraps a typed error. msg := strings.ToLower(err.Error()) - return strings.Contains(msg, "not found") || strings.Contains(msg, "404") + return strings.Contains(msg, "not found") } // Resolve returns the manifest entry for the given CLI version. @@ -241,6 +288,7 @@ func manifestLocalPath(ctx context.Context) string { } home, err := os.UserCacheDir() if err != nil { + log.Debugf(ctx, "Could not determine user cache directory: %v", err) return "" } return filepath.Join(home, "databricks", localManifestFile) @@ -267,8 +315,7 @@ func readLocalManifest(path string) (cachedManifest, error) { } // writeLocalManifest writes the manifest to the local cache path using a -// temp-file-then-rename pattern. The os.Remove before os.Rename is needed -// for Windows, where Rename fails if the destination exists. +// temp-file-then-rename pattern for atomicity. func writeLocalManifest(ctx context.Context, path string, m Manifest) { if path == "" { return @@ -280,12 +327,12 @@ func writeLocalManifest(ctx context.Context, path string, m Manifest) { } dir := filepath.Dir(path) if err := os.MkdirAll(dir, 0o700); err != nil { - log.Debugf(ctx, "Failed to create cache directory %s: %v", dir, err) + log.Warnf(ctx, "Failed to create cache directory %s: %v", dir, err) return } tmp, err := os.CreateTemp(dir, ".compat-manifest-*.tmp") if err != nil { - log.Debugf(ctx, "Failed to create temp cache file: %v", err) + log.Warnf(ctx, "Failed to create temp cache file: %v", err) return } tmpPath := tmp.Name() @@ -301,20 +348,20 @@ func writeLocalManifest(ctx context.Context, path string, m Manifest) { log.Debugf(ctx, "Failed to close temp cache file: %v", err) return } - _ = os.Remove(path) if err := os.Rename(tmpPath, path); err != nil { - log.Debugf(ctx, "Failed to rename temp cache file: %v", err) + log.Warnf(ctx, "Failed to rename temp cache file: %v", err) } } // --- Remote fetch --- -// fetchRemoteWithRetry wraps fetchRemote with retries. +// fetchRemoteWithRetry wraps fetchRemote with retries on transient errors. +// Client errors (4xx) are not retried since they will not recover. func fetchRemoteWithRetry(ctx context.Context) (Manifest, error) { var lastErr error - for attempt := range fetchRetries + 1 { + for attempt := range maxFetchAttempts { if attempt > 0 { - log.Debugf(ctx, "Retrying manifest fetch (attempt %d)", attempt+1) + log.Debugf(ctx, "Retrying manifest fetch (attempt %d/%d)", attempt+1, maxFetchAttempts) select { case <-ctx.Done(): return nil, ctx.Err() @@ -326,6 +373,12 @@ func fetchRemoteWithRetry(ctx context.Context) (Manifest, error) { return m, nil } lastErr = err + + // Do not retry client errors (4xx) — they won't resolve on retry. + var httpErr *HTTPStatusError + if errors.As(err, &httpErr) && httpErr.StatusCode >= 400 && httpErr.StatusCode < 500 { + return nil, lastErr + } } return nil, lastErr } @@ -336,6 +389,7 @@ func fetchRemote(ctx context.Context) (Manifest, error) { if err != nil { return nil, err } + req.Header.Set("User-Agent", "databricks-cli/"+build.GetInfo().Version) resp, err := httpClient.Do(req) if err != nil { @@ -344,7 +398,7 @@ func fetchRemote(ctx context.Context) (Manifest, error) { defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("HTTP %d fetching manifest", resp.StatusCode) + return nil, &HTTPStatusError{StatusCode: resp.StatusCode} } // Cap response size to guard against corrupted or malicious responses. @@ -372,5 +426,19 @@ func parseManifest(data []byte) (Manifest, error) { return nil, fmt.Errorf("invalid semver key %q in compatibility manifest", k) } } + for k, entry := range m { + if entry.AppKit == "" { + return nil, fmt.Errorf("manifest entry %q has empty appkit version", k) + } + if entry.AgentSkills == "" { + return nil, fmt.Errorf("manifest entry %q has empty skills version", k) + } + if !semver.IsValid("v" + entry.AppKit) { + return nil, fmt.Errorf("manifest entry %q has invalid appkit version %q", k, entry.AppKit) + } + if !semver.IsValid("v" + entry.AgentSkills) { + return nil, fmt.Errorf("manifest entry %q has invalid skills version %q", k, entry.AgentSkills) + } + } return m, nil } diff --git a/libs/clicompat/clicompat_test.go b/libs/clicompat/clicompat_test.go index f40f64dd80..a2351254c6 100644 --- a/libs/clicompat/clicompat_test.go +++ b/libs/clicompat/clicompat_test.go @@ -3,6 +3,9 @@ package clicompat import ( "context" "encoding/json" + "errors" + "fmt" + "io/fs" "net/http" "net/http/httptest" "net/url" @@ -431,3 +434,117 @@ func TestReadWriteLocalManifest(t *testing.T) { assert.Equal(t, "0.50.0", cached.manifest["next"].AppKit) assert.True(t, cached.isFresh(cacheTTL), "just-written file should be fresh") } + +// --- IsNotFoundError tests --- + +func TestIsNotFoundError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {name: "nil", err: nil, want: false}, + {name: "unrelated error", err: errors.New("connection refused"), want: false}, + {name: "sentinel ErrNotFound", err: ErrNotFound, want: true}, + {name: "wrapped ErrNotFound", err: fmt.Errorf("fetching: %w", ErrNotFound), want: true}, + {name: "HTTPStatusError 404", err: &HTTPStatusError{StatusCode: 404}, want: true}, + {name: "wrapped HTTPStatusError 404", err: fmt.Errorf("fetch failed: %w", &HTTPStatusError{StatusCode: 404}), want: true}, + {name: "HTTPStatusError 500", err: &HTTPStatusError{StatusCode: 500}, want: false}, + {name: "HTTPStatusError 403", err: &HTTPStatusError{StatusCode: 403}, want: false}, + {name: "git not found", err: errors.New("Remote branch template-v99 not found in upstream origin"), want: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, IsNotFoundError(tc.err)) + }) + } +} + +// --- parseManifest entry validation tests --- + +func TestParseManifest_EmptyAppKit(t *testing.T) { + data := `{"next":{"appkit":"","skills":"0.1.5"}}` + _, err := parseManifest([]byte(data)) + require.Error(t, err) + assert.Contains(t, err.Error(), "empty appkit version") +} + +func TestParseManifest_EmptySkills(t *testing.T) { + data := `{"next":{"appkit":"0.27.0","skills":""}}` + _, err := parseManifest([]byte(data)) + require.Error(t, err) + assert.Contains(t, err.Error(), "empty skills version") +} + +func TestParseManifest_InvalidAppKitSemver(t *testing.T) { + data := `{"next":{"appkit":"not-semver","skills":"0.1.5"}}` + _, err := parseManifest([]byte(data)) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid appkit version") +} + +func TestParseManifest_InvalidSkillsSemver(t *testing.T) { + data := `{"next":{"appkit":"0.27.0","skills":"abc"}}` + _, err := parseManifest([]byte(data)) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid skills version") +} + +// --- FetchManifest no-retry-on-404 test --- + +func TestFetchManifest_NoRetryOn404(t *testing.T) { + ctx := testContext(t) + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + redirectToServer(t, srv) + + // Should fall back to embedded manifest (since 404 is not retried). + result, err := FetchManifest(ctx) + require.NoError(t, err) + + embedded, _ := parseEmbeddedManifest() + assert.Equal(t, embedded["next"].AppKit, result["next"].AppKit) + assert.Equal(t, int32(1), callCount.Load(), "404 should not be retried") +} + +// --- FetchManifest cache-disabled test --- + +func TestFetchManifest_CacheDisabled(t *testing.T) { + ctx := testContext(t) + ctx = env.Set(ctx, "DATABRICKS_CACHE_ENABLED", "false") + + want := Manifest{ + "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, + } + body, _ := json.Marshal(want) + + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + _, _ = w.Write(body) + })) + defer srv.Close() + redirectToServer(t, srv) + + // First call fetches from remote. + result1, err := FetchManifest(ctx) + require.NoError(t, err) + assert.Equal(t, "0.99.0", result1["next"].AppKit) + + // Second call should also fetch from remote (cache is disabled). + result2, err := FetchManifest(ctx) + require.NoError(t, err) + assert.Equal(t, "0.99.0", result2["next"].AppKit) + + assert.Equal(t, int32(2), callCount.Load(), "with cache disabled, both calls should hit the server") + + // Verify no cache file was written. + localPath := manifestLocalPath(ctx) + _, statErr := os.Stat(localPath) + assert.ErrorIs(t, statErr, fs.ErrNotExist, "cache file should not exist when cache is disabled") +} From 847438fdefb5a8ef3fc7cb79d8e2632687213fdc Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Mon, 11 May 2026 11:47:41 +0200 Subject: [PATCH 08/10] fix: improve fallback guards for AppKit and skills resolution - Only fall back to embedded version when version was auto-resolved, not when user explicitly passed --version or --branch - Skip fallback if embedded version matches the one already tried - Log warning when embedded fallback resolution itself fails Co-authored-by: Isaac --- cmd/apps/init.go | 11 +++++++---- cmd/apps/manifest.go | 7 +++++-- experimental/aitools/lib/installer/installer.go | 2 ++ 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/cmd/apps/init.go b/cmd/apps/init.go index d7844cce10..abbe40828e 100644 --- a/cmd/apps/init.go +++ b/cmd/apps/init.go @@ -863,15 +863,18 @@ func runCreate(ctx context.Context, opts createOptions) error { // Step 2: Wait for template (may already be done if the user took time typing the name) resolvedPath, cleanup, err := awaitTemplate(ctx, templateCh) - if err != nil && usingDefaultTemplate && clicompat.IsNotFoundError(err) { - // The resolved version doesn't exist as a tag. Fall back to the - // embedded manifest which ships a known-good version. + // Only fall back to the embedded version when the version was auto-resolved + // from the manifest, not when the user explicitly passed --version or --branch. + versionAutoResolved := opts.version == "" && opts.branch == "" + if err != nil && usingDefaultTemplate && versionAutoResolved && clicompat.IsNotFoundError(err) { fallbackVersion, fbErr := clicompat.ResolveEmbeddedAppKitVersion() - if fbErr == nil && fallbackVersion != "" { + if fbErr == nil && fallbackVersion != "" && normalizeVersion(fallbackVersion) != branchForClone { log.Warnf(ctx, "Template version not found, falling back to embedded version %s", fallbackVersion) fallbackRef := normalizeVersion(fallbackVersion) templateCh = resolveTemplateAsync(ctx, templateSrc, fallbackRef, appkitTemplateDir) resolvedPath, cleanup, err = awaitTemplate(ctx, templateCh) + } else if fbErr != nil { + log.Warnf(ctx, "Could not resolve embedded AppKit version: %v", fbErr) } } if err != nil { diff --git a/cmd/apps/manifest.go b/cmd/apps/manifest.go index b79e04c6ff..2fcc29099f 100644 --- a/cmd/apps/manifest.go +++ b/cmd/apps/manifest.go @@ -45,12 +45,15 @@ func runManifestOnly(ctx context.Context, templatePath, branch, version string) subdirForClone = appkitTemplateDir } resolvedPath, cleanup, err := resolveTemplate(ctx, templateSrc, branchForClone, subdirForClone) - if err != nil && usingDefaultTemplate && clicompat.IsNotFoundError(err) { + versionAutoResolved := version == "" && branch == "" + if err != nil && usingDefaultTemplate && versionAutoResolved && clicompat.IsNotFoundError(err) { fallbackVersion, fbErr := clicompat.ResolveEmbeddedAppKitVersion() - if fbErr == nil && fallbackVersion != "" { + if fbErr == nil && fallbackVersion != "" && normalizeVersion(fallbackVersion) != gitRef { log.Warnf(ctx, "Template version not found, falling back to embedded version %s", fallbackVersion) fallbackRef := normalizeVersion(fallbackVersion) resolvedPath, cleanup, err = resolveTemplate(ctx, templateSrc, fallbackRef, appkitTemplateDir) + } else if fbErr != nil { + log.Warnf(ctx, "Could not resolve embedded AppKit version: %v", fbErr) } } if err != nil { diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 5bdeb871bb..504345369c 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -106,6 +106,8 @@ func FetchSkillsManifestWithFallback(ctx context.Context, src ManifestSource, re log.Warnf(ctx, "Skills version %s not found, falling back to embedded version %s", tag, fallbackVersion) ref = "v" + fallbackVersion manifest, err = src.FetchManifest(ctx, ref) + } else if fbErr != nil { + log.Warnf(ctx, "Could not resolve embedded skills version: %v", fbErr) } } return manifest, ref, err From 149178314f8b09d1c9ebc818ea6750efca4c3583 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Mon, 11 May 2026 13:23:53 +0200 Subject: [PATCH 09/10] fix: skip skills fallback when DATABRICKS_SKILLS_REF is explicitly set GetSkillsRef now returns whether the ref was explicitly set via env var. FetchSkillsManifestWithFallback accepts allowFallback to skip the embedded fallback when the user explicitly chose a ref. Co-authored-by: Isaac --- experimental/aitools/cmd/list.go | 4 ++-- experimental/aitools/cmd/version.go | 2 +- .../aitools/lib/installer/installer.go | 19 ++++++++++--------- .../aitools/lib/installer/installer_test.go | 3 ++- experimental/aitools/lib/installer/update.go | 4 ++-- 5 files changed, 17 insertions(+), 15 deletions(-) diff --git a/experimental/aitools/cmd/list.go b/experimental/aitools/cmd/list.go index 56d323a142..90d397aba6 100644 --- a/experimental/aitools/cmd/list.go +++ b/experimental/aitools/cmd/list.go @@ -48,13 +48,13 @@ func newListCmd() *cobra.Command { func defaultListSkills(cmd *cobra.Command, scope string) error { ctx := cmd.Context() - ref, err := installer.GetSkillsRef(ctx) + ref, explicit, err := installer.GetSkillsRef(ctx) if err != nil { return err } src := &installer.GitHubManifestSource{} - manifest, ref, err := installer.FetchSkillsManifestWithFallback(ctx, src, ref) + manifest, ref, err := installer.FetchSkillsManifestWithFallback(ctx, src, ref, !explicit) if err != nil { return fmt.Errorf("failed to fetch manifest: %w", err) } diff --git a/experimental/aitools/cmd/version.go b/experimental/aitools/cmd/version.go index a5bde04e95..fd3d086e95 100644 --- a/experimental/aitools/cmd/version.go +++ b/experimental/aitools/cmd/version.go @@ -45,7 +45,7 @@ func newVersionCmd() *cobra.Command { return nil } - latestRef, err := installer.GetSkillsRef(ctx) + latestRef, _, err := installer.GetSkillsRef(ctx) if err != nil { log.Debugf(ctx, "could not resolve skills version: %v", err) } diff --git a/experimental/aitools/lib/installer/installer.go b/experimental/aitools/lib/installer/installer.go index 504345369c..dbf511720b 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -33,17 +33,18 @@ const ( // It is a package-level var so tests can replace it with a mock. var fetchFileFn = fetchSkillFile -// GetSkillsRef returns the skills repo ref to use. +// GetSkillsRef returns the skills repo ref to use and whether it was explicitly +// set via DATABRICKS_SKILLS_REF (as opposed to auto-resolved from the manifest). // Resolution order: DATABRICKS_SKILLS_REF env var → compatibility manifest → error. -func GetSkillsRef(ctx context.Context) (string, error) { +func GetSkillsRef(ctx context.Context) (ref string, explicit bool, err error) { if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" { - return ref, nil + return ref, true, nil } v, err := clicompat.ResolveAgentSkillsVersion(ctx) if err != nil { - return "", fmt.Errorf("could not resolve skills version: %w", err) + return "", false, fmt.Errorf("could not resolve skills version: %w", err) } - return "v" + v, nil + return "v" + v, false, nil } // Manifest describes the skills manifest fetched from the skills repo. @@ -97,10 +98,10 @@ func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byt // If the ref points to a non-existent tag (not-found error), it falls back to // the embedded manifest's skills version. Returns the manifest, the (possibly // updated) ref, and any error. -func FetchSkillsManifestWithFallback(ctx context.Context, src ManifestSource, ref string) (*Manifest, string, error) { +func FetchSkillsManifestWithFallback(ctx context.Context, src ManifestSource, ref string, allowFallback bool) (*Manifest, string, error) { tag := strings.TrimPrefix(ref, "v") manifest, err := src.FetchManifest(ctx, ref) - if err != nil && clicompat.IsNotFoundError(err) { + if err != nil && allowFallback && clicompat.IsNotFoundError(err) { fallbackVersion, fbErr := clicompat.ResolveEmbeddedAgentSkillsVersion() if fbErr == nil && fallbackVersion != "" && fallbackVersion != tag { log.Warnf(ctx, "Skills version %s not found, falling back to embedded version %s", tag, fallbackVersion) @@ -117,12 +118,12 @@ func FetchSkillsManifestWithFallback(ctx context.Context, src ManifestSource, re // This is the core installation function. Callers are responsible for agent detection, // prompting, and printing the "Installing..." header. func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgents []*agents.Agent, opts InstallOptions) error { - ref, err := GetSkillsRef(ctx) + ref, explicit, err := GetSkillsRef(ctx) if err != nil { return err } cmdio.LogString(ctx, "Using skills version "+strings.TrimPrefix(ref, "v")) - manifest, ref, err := FetchSkillsManifestWithFallback(ctx, src, ref) + manifest, ref, err := FetchSkillsManifestWithFallback(ctx, src, ref, !explicit) if err != nil { return err } diff --git a/experimental/aitools/lib/installer/installer_test.go b/experimental/aitools/lib/installer/installer_test.go index 848ab54172..ab64f8605b 100644 --- a/experimental/aitools/lib/installer/installer_test.go +++ b/experimental/aitools/lib/installer/installer_test.go @@ -753,8 +753,9 @@ func TestGetSkillsRefResolvesFromManifest(t *testing.T) { manifest := `{"next":{"appkit":"0.24.0","skills":"0.1.5"},"0.300.0":{"appkit":"0.24.0","skills":"0.1.5"}}` require.NoError(t, os.WriteFile(cachePath, []byte(manifest), 0o644)) - ref, err := GetSkillsRef(t.Context()) + ref, explicit, err := GetSkillsRef(t.Context()) require.NoError(t, err, "GetSkillsRef should succeed via cached manifest") + assert.False(t, explicit, "ref resolved from manifest should not be explicit") assert.NotEmpty(t, ref) assert.True(t, len(ref) > 1 && ref[0] == 'v', "ref should start with 'v', got %q", ref) } diff --git a/experimental/aitools/lib/installer/update.go b/experimental/aitools/lib/installer/update.go index ea3674116b..720359d334 100644 --- a/experimental/aitools/lib/installer/update.go +++ b/experimental/aitools/lib/installer/update.go @@ -81,7 +81,7 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent return nil, errors.New("no skills installed. Run 'databricks experimental aitools install' to install") } - latestTag, err := GetSkillsRef(ctx) + latestTag, explicit, err := GetSkillsRef(ctx) if err != nil { return nil, err } @@ -91,7 +91,7 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent return &UpdateResult{Unchanged: slices.Sorted(maps.Keys(state.Skills))}, nil } - manifest, latestTag, err := FetchSkillsManifestWithFallback(ctx, src, latestTag) + manifest, latestTag, err := FetchSkillsManifestWithFallback(ctx, src, latestTag, !explicit) if err != nil { if opts.Check { log.Warnf(ctx, "Could not fetch manifest: %v", err) From ea39588d16f73cb1209e00c35bcf6a9b74d3f838 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 12 May 2026 16:21:41 +0200 Subject: [PATCH 10/10] refactor: remove "next" key from compat manifest, add DATABRICKS_FORCE_EMBEDDED_COMPAT The manifest is now purely range-based: each versioned entry defines a range floor that applies to that CLI version and all above it. The "next" key was redundant since we always know the CLI version when updating the manifest. Dev builds now resolve to the highest versioned entry. Also adds DATABRICKS_FORCE_EMBEDDED_COMPAT=true env var to skip remote fetch and use only the embedded manifest, useful for local development. Co-authored-by: Isaac --- .agent/skills/bump-cli-compat/SKILL.md | 25 ++- internal/build/README.md | 36 ++-- internal/build/cli-compat.json | 1 - libs/clicompat/clicompat.go | 70 ++++---- libs/clicompat/clicompat_test.go | 224 ++++++++++--------------- 5 files changed, 168 insertions(+), 188 deletions(-) diff --git a/.agent/skills/bump-cli-compat/SKILL.md b/.agent/skills/bump-cli-compat/SKILL.md index a1fe768776..a0612a2ba0 100644 --- a/.agent/skills/bump-cli-compat/SKILL.md +++ b/.agent/skills/bump-cli-compat/SKILL.md @@ -66,9 +66,30 @@ If any tag doesn't exist, report the error and stop. Read `internal/build/cli-compat.json`. Note the current versions and the list of versioned entries. -### Step 4: Update the manifest +### Step 4: Determine update type -Update **all entries** (both `next` and all versioned CLI entries) to the new appkit and skills versions. This is the "no template changes" scenario — a simple search & replace. +Ask the user: + +> Do any of these apply? +> - **AppKit**: The new templates require new CLI logic in `apps init` (e.g. new flags, prompts, or template handling that older CLIs don't have) +> - **Skills**: The new skills version uses CLI commands that older CLIs don't support +> +> If **neither** applies, this is a non-breaking bump (default). + +- **No breaking changes** (default): proceed to Step 4a. +- **Breaking changes**: proceed to Step 4b. + +### Step 4a: No breaking changes (update in-place) + +Update the **highest versioned entry** to the new appkit and skills versions. Do NOT add new versioned keys. The manifest is range-based: updating the highest entry automatically covers all CLI versions in that range. + +Write the updated `internal/build/cli-compat.json`. + +### Step 4b: Breaking changes (add new entry) + +Ask the user for the **minimum CLI version** that supports the new features. + +Add a new entry keyed to that CLI version with the new appkit and skills versions. Keep older entries unchanged so older CLI binaries stay compatible. Write the updated `internal/build/cli-compat.json`. diff --git a/internal/build/README.md b/internal/build/README.md index 82e3b5df9d..913ae7e5f4 100644 --- a/internal/build/README.md +++ b/internal/build/README.md @@ -6,22 +6,27 @@ ```json { - "next": { "appkit": "0.24.0", "skills": "0.1.5" }, - "0.300.0": { "appkit": "0.24.0", "skills": "0.1.5" } + "0.296.0": { "appkit": "0.27.0", "skills": "0.1.5" }, + "0.290.0": { "appkit": "0.24.0", "skills": "0.1.4" }, + "0.280.0": { "appkit": "0.20.0", "skills": "0.1.0" } } ``` -- Each key is a CLI version (`X.Y.Z`) or `"next"`. -- Each value specifies the compatible `appkit` and `skills` versions. -- `"next"` is used for dev builds (`0.0.0-dev*`). For production CLI versions newer than all listed entries, the highest versioned entry is used. +Each key is a CLI version in semver format. Each entry defines a **range floor**: it applies to that CLI version and all versions above it, up to (but not including) the next entry. The manifest should be **sparse** — not every CLI version needs its own entry. Only add a new entry when a compatibility boundary changes. + +For example, with the manifest above: +- CLI `0.285.0` → uses `0.280.0` entry (appkit `0.20.0`) +- CLI `0.293.0` → uses `0.290.0` entry (appkit `0.24.0`) +- CLI `0.300.0` → uses `0.296.0` entry (appkit `0.27.0`, highest versioned) +- CLI `0.0.0-dev+abc` → uses `0.296.0` entry (dev builds use the highest versioned entry) ## How the CLI resolves versions -1. **Exact match** on CLI version → use that entry. -2. **No exact match**, between two entries → use the nearest lower version's entry. -3. **Newer than all entries** → use the highest versioned entry. -4. **Older than all entries** → use the lowest (oldest) entry. -5. **Dev builds** (`0.0.0-dev*`) → use `"next"`. +1. **Dev builds** (`0.0.0-dev*`) → use the highest versioned entry. +2. **Exact match** on CLI version → use that entry. +3. **No exact match**, between two entries → use the nearest lower version's entry. +4. **Newer than all entries** → use the highest versioned entry. +5. **Older than all entries** → use the lowest (oldest) entry. ## Manifest sources (fallback chain) @@ -32,15 +37,16 @@ At runtime, the CLI resolves the manifest from four sources: 3. **Stale local cache** — if remote fetch fails but a previously cached file exists (even if expired), it is used as-is. 4. **Embedded manifest** — compiled into the binary via `go:embed`. Used as last resort when both remote and local cache fail. +Set `DATABRICKS_FORCE_EMBEDDED_COMPAT=true` to skip all tiers and use only the embedded manifest. This is useful for local development when testing with a locally compiled binary that has a modified `cli-compat.json`. + ## When to update -After each AppKit or Agent Skills release: +The goal is to **keep the manifest sparse** — only add entries at compatibility boundaries. After each AppKit or Agent Skills release: 1. **Run evals** on the new AppKit version. If there is no regression, proceed. 2. **Open a PR** to update `cli-compat.json`. The change depends on the type of release: - - **No template changes** (just an AppKit/skills version bump): search & replace all version occurrences in the manifest and update `next`. - - **Template changes that don't require new CLI features**: test the last 3 CLI versions with the new template and update matching entries. - - **Template changes that require new CLI features**: add a new entry for the minimum CLI version that supports them; older entries keep pointing to the previous template version. + - **No breaking changes** (the new AppKit/skills version works with all existing CLI versions): update the existing highest versioned entry's appkit/skills values in-place. Do NOT add a new versioned key. All CLI versions in that range automatically pick up the new versions. + - **Breaking changes** (the new AppKit templates require specific `apps init` features, or the new skills version requires CLI commands that older CLIs lack): add a new entry keyed to the **minimum CLI version** that supports the new features. Older entries keep their previous appkit/skills values so older CLI binaries stay compatible. This process is manual for now but can be automated as part of the release workflow in the future. Use the `/bump-cli-compat` Claude Code skill to automate the update and PR creation. @@ -52,7 +58,7 @@ The manifest is validated by Go tests in `libs/clicompat/`: go test ./libs/clicompat/... -run TestEmbeddedManifest -v ``` -This checks: valid JSON, `"next"` key present, at least one versioned entry, valid semver keys, valid semver entry values, `next` versions >= all entries, and ascending key order. +This checks: valid JSON, at least one entry, valid semver keys, valid semver entry values, and ascending key order. ## Pruning policy diff --git a/internal/build/cli-compat.json b/internal/build/cli-compat.json index 33fc41f0a6..d3c37c23be 100644 --- a/internal/build/cli-compat.json +++ b/internal/build/cli-compat.json @@ -1,4 +1,3 @@ { - "next": { "appkit": "0.24.0", "skills": "0.1.5" }, "0.300.0": { "appkit": "0.24.0", "skills": "0.1.5" } } diff --git a/libs/clicompat/clicompat.go b/libs/clicompat/clicompat.go index 0ffe76e482..8d982db132 100644 --- a/libs/clicompat/clicompat.go +++ b/libs/clicompat/clicompat.go @@ -41,16 +41,15 @@ const ( // fetchTimeout is the HTTP timeout for fetching the manifest at runtime. fetchTimeout = 3 * time.Second - // nextKey is the special manifest key for CLI versions newer than any entry. - nextKey = "next" - // cacheTTL is how long a locally cached manifest is considered fresh. cacheTTL = 1 * time.Hour // localManifestFile is the filename for the locally cached manifest. localManifestFile = "compat-manifest.json" - // devVersionPrefix identifies dev builds that always use the "next" entry. + // devVersionPrefix identifies dev builds whose semver (0.0.0) is lower than + // all real CLI versions. These are treated as bleeding-edge and resolve to + // the highest versioned entry. devVersionPrefix = "0.0.0-dev" maxFetchAttempts = 3 @@ -88,9 +87,18 @@ var httpClient = &http.Client{Timeout: fetchTimeout} // 3. Stale local file (if remote fails but a previously cached file exists) // 4. Embedded manifest compiled into the binary // +// Set DATABRICKS_FORCE_EMBEDDED_COMPAT=true to skip all tiers and use only +// the embedded manifest. Useful for local development when testing with a +// locally compiled binary. +// // Set DATABRICKS_CACHE_ENABLED=false to bypass the local cache (tiers 1 and 3a), // which is useful to recover from a bad cached manifest. func FetchManifest(ctx context.Context) (Manifest, error) { + if force, ok := env.GetBool(ctx, "DATABRICKS_FORCE_EMBEDDED_COMPAT"); ok && force { + log.Debugf(ctx, "Using embedded manifest (DATABRICKS_FORCE_EMBEDDED_COMPAT=true)") + return parseEmbeddedManifest() + } + cacheEnabled := true if enabled, ok := env.GetBool(ctx, "DATABRICKS_CACHE_ENABLED"); ok { cacheEnabled = enabled @@ -197,8 +205,11 @@ func IsNotFoundError(err error) bool { // Resolve returns the manifest entry for the given CLI version. // +// Each versioned entry defines a range floor: it applies to that CLI version +// and all versions above it, up to (but not including) the next entry. +// // Resolution order: -// 1. Dev builds (version starts with "0.0.0-dev") use the "next" entry. +// 1. Dev builds (version starts with "0.0.0-dev") use the highest versioned entry. // 2. Exact match on CLI version. // 3. Nearest lower version (semver-sorted). This also handles CLI versions // newer than all entries, returning the highest known entry. @@ -208,14 +219,17 @@ func Resolve(m Manifest, cliVersion string) (Entry, error) { return Entry{}, errors.New("empty compatibility manifest") } - next, ok := m[nextKey] - if !ok { - return Entry{}, fmt.Errorf("compatibility manifest missing %q key", nextKey) + // Collect and sort versioned keys descending. + versions := sortedVersions(m) + if len(versions) == 0 { + return Entry{}, errors.New("compatibility manifest has no versioned entries") } - // Dev builds always use "next". + // Dev builds (0.0.0-dev*) have semver lower than all real CLI versions, + // so they would incorrectly resolve to the lowest entry. Use the highest + // versioned entry instead, since dev builds represent the bleeding edge. if strings.HasPrefix(cliVersion, devVersionPrefix) { - return next, nil + return m[versions[0]], nil } // Exact match. @@ -223,19 +237,6 @@ func Resolve(m Manifest, cliVersion string) (Entry, error) { return entry, nil } - // Collect and sort versioned keys (exclude "next"). - var versions []string - for k := range m { - if k != nextKey { - versions = append(versions, k) - } - } - - // Sort descending by semver. The semver package requires a "v" prefix. - slices.SortFunc(versions, func(a, b string) int { - return semver.Compare("v"+b, "v"+a) - }) - // Find the nearest lower version. vCLI := "v" + cliVersion for _, v := range versions { @@ -244,14 +245,22 @@ func Resolve(m Manifest, cliVersion string) (Entry, error) { } } - // CLI is older than all entries — use the lowest (oldest) entry as closest match. - // If there are no versioned entries (only "next"), fall back to "next". - if len(versions) == 0 { - return next, nil - } + // CLI is older than all entries — use the lowest (oldest) entry. return m[versions[len(versions)-1]], nil } +// sortedVersions returns manifest keys sorted descending by semver. +func sortedVersions(m Manifest) []string { + versions := make([]string, 0, len(m)) + for k := range m { + versions = append(versions, k) + } + slices.SortFunc(versions, func(a, b string) int { + return semver.Compare("v"+b, "v"+a) + }) + return versions +} + // resolveEntry fetches the manifest, resolves for the given CLI version. func resolveEntry(ctx context.Context, cliVersion string) (Entry, error) { m, err := FetchManifest(ctx) @@ -418,11 +427,8 @@ func parseManifest(data []byte) (Manifest, error) { if len(m) == 0 { return nil, errors.New("empty compatibility manifest") } - if _, ok := m[nextKey]; !ok { - return nil, fmt.Errorf("compatibility manifest missing %q key", nextKey) - } for k := range m { - if k != nextKey && !semver.IsValid("v"+k) { + if !semver.IsValid("v" + k) { return nil, fmt.Errorf("invalid semver key %q in compatibility manifest", k) } } diff --git a/libs/clicompat/clicompat_test.go b/libs/clicompat/clicompat_test.go index a2351254c6..87d29c6dfe 100644 --- a/libs/clicompat/clicompat_test.go +++ b/libs/clicompat/clicompat_test.go @@ -53,78 +53,45 @@ func testContext(t *testing.T) context.Context { return env.Set(t.Context(), "DATABRICKS_CACHE_DIR", t.TempDir()) } -func testManifest() Manifest { - return Manifest{ - "next": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, - "0.296.0": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, - "0.295.0": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, - "0.290.0": {AppKit: "0.24.0", AgentSkills: "0.1.5"}, - "0.288.0": {AppKit: "0.24.0", AgentSkills: "0.1.4"}, - } -} - -// --- Resolve tests (unchanged, no network) --- - -func TestResolve_ExactMatch(t *testing.T) { - m := testManifest() - entry, err := Resolve(m, "0.296.0") - require.NoError(t, err) - assert.Equal(t, "0.27.0", entry.AppKit) - assert.Equal(t, "0.1.5", entry.AgentSkills) -} - -func TestResolve_NearestLower(t *testing.T) { - m := testManifest() - entry, err := Resolve(m, "0.293.0") - require.NoError(t, err) - assert.Equal(t, "0.24.0", entry.AppKit) - assert.Equal(t, "0.1.5", entry.AgentSkills) -} - -func TestResolve_NewerThanAll(t *testing.T) { +// --- Resolve tests --- + +// TestResolve_Ranges verifies range-based resolution. Each versioned entry +// defines a range floor: it applies to that CLI version and all versions above +// it up to (but not including) the next entry. Dev builds use the highest +// versioned entry. The manifest uses distinct appkit values so assertions are +// unambiguous. +func TestResolve_Ranges(t *testing.T) { m := Manifest{ - "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, "0.296.0": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, - "0.290.0": {AppKit: "0.24.0", AgentSkills: "0.1.5"}, + "0.290.0": {AppKit: "0.24.0", AgentSkills: "0.1.4"}, + "0.280.0": {AppKit: "0.20.0", AgentSkills: "0.1.0"}, } - entry, err := Resolve(m, "0.300.0") - require.NoError(t, err) - assert.Equal(t, "0.27.0", entry.AppKit) - assert.Equal(t, "0.1.5", entry.AgentSkills) -} - -func TestResolve_DevBuild(t *testing.T) { - m := testManifest() - entry, err := Resolve(m, "0.0.0-dev+abc123def") - require.NoError(t, err) - assert.Equal(t, "0.27.0", entry.AppKit) - assert.Equal(t, "0.1.5", entry.AgentSkills) -} - -func TestResolve_OlderThanAll(t *testing.T) { - m := testManifest() - entry, err := Resolve(m, "0.280.0") - require.NoError(t, err) - assert.Equal(t, "0.24.0", entry.AppKit) - assert.Equal(t, "0.1.4", entry.AgentSkills) -} -func TestResolve_OnlyNextKey(t *testing.T) { - m := Manifest{ - "next": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, + tests := []struct { + name string + cliVersion string + wantAppKit string + wantSkills string + }{ + {"exact match at range floor", "0.280.0", "0.20.0", "0.1.0"}, + {"mid-range", "0.285.0", "0.20.0", "0.1.0"}, + {"just below next range", "0.289.9", "0.20.0", "0.1.0"}, + {"exact match mid entry", "0.290.0", "0.24.0", "0.1.4"}, + {"between mid and top", "0.293.0", "0.24.0", "0.1.4"}, + {"exact match highest", "0.296.0", "0.27.0", "0.1.5"}, + {"newer than all entries uses highest", "0.300.0", "0.27.0", "0.1.5"}, + {"older than all entries uses lowest", "0.270.0", "0.20.0", "0.1.0"}, + {"dev build uses highest", "0.0.0-dev+abc123", "0.27.0", "0.1.5"}, + {"bare dev uses highest", "0.0.0-dev", "0.27.0", "0.1.5"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + entry, err := Resolve(m, tc.cliVersion) + require.NoError(t, err) + assert.Equal(t, tc.wantAppKit, entry.AppKit) + assert.Equal(t, tc.wantSkills, entry.AgentSkills) + }) } - entry, err := Resolve(m, "0.296.0") - require.NoError(t, err) - assert.Equal(t, "0.27.0", entry.AppKit) - assert.Equal(t, "0.1.5", entry.AgentSkills) -} - -func TestResolve_LowestEntryExactMatch(t *testing.T) { - m := testManifest() - entry, err := Resolve(m, "0.288.0") - require.NoError(t, err) - assert.Equal(t, "0.24.0", entry.AppKit) - assert.Equal(t, "0.1.4", entry.AgentSkills) } func TestResolve_EmptyManifest(t *testing.T) { @@ -134,21 +101,11 @@ func TestResolve_EmptyManifest(t *testing.T) { assert.Contains(t, err.Error(), "empty compatibility manifest") } -func TestResolve_MissingNextKey(t *testing.T) { - m := Manifest{ - "0.296.0": {AppKit: "0.27.0", AgentSkills: "0.1.5"}, - } - _, err := Resolve(m, "0.296.0") - assert.Error(t, err) - assert.Contains(t, err.Error(), `missing "next" key`) -} - // --- FetchManifest tests --- func TestFetchManifest_RemoteSuccess(t *testing.T) { ctx := testContext(t) want := Manifest{ - "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, } body, _ := json.Marshal(want) @@ -164,7 +121,7 @@ func TestFetchManifest_RemoteSuccess(t *testing.T) { result, err := FetchManifest(ctx) require.NoError(t, err) assert.True(t, called, "test server should have been called") - assert.Equal(t, "0.99.0", result["next"].AppKit) + assert.Equal(t, "0.99.0", result["0.296.0"].AppKit) } func TestFetchManifest_RemoteFailFallsBackToEmbedded(t *testing.T) { @@ -181,7 +138,7 @@ func TestFetchManifest_RemoteFailFallsBackToEmbedded(t *testing.T) { // Verify it returned the embedded manifest values. embedded, _ := parseManifest(build.CLICompatManifestJSON) - assert.Equal(t, embedded["next"].AppKit, result["next"].AppKit) + assert.Equal(t, embedded["0.300.0"].AppKit, result["0.300.0"].AppKit) } func TestFetchManifest_RemoteFailFallsBackToStaleCache(t *testing.T) { @@ -189,7 +146,6 @@ func TestFetchManifest_RemoteFailFallsBackToStaleCache(t *testing.T) { // Pre-populate the local cache with a stale manifest. staleManifest := Manifest{ - "next": {AppKit: "0.88.0", AgentSkills: "0.8.8"}, "0.296.0": {AppKit: "0.88.0", AgentSkills: "0.8.8"}, } localPath := manifestLocalPath(ctx) @@ -207,13 +163,12 @@ func TestFetchManifest_RemoteFailFallsBackToStaleCache(t *testing.T) { result, err := FetchManifest(ctx) require.NoError(t, err) // Should return the stale cached manifest, not the embedded one. - assert.Equal(t, "0.88.0", result["next"].AppKit) + assert.Equal(t, "0.88.0", result["0.296.0"].AppKit) } func TestFetchManifest_RemoteSuccessWritesLocalCache(t *testing.T) { ctx := testContext(t) want := Manifest{ - "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, } body, _ := json.Marshal(want) @@ -236,7 +191,6 @@ func TestFetchManifest_RemoteSuccessWritesLocalCache(t *testing.T) { func TestFetchManifest_CacheHit(t *testing.T) { ctx := testContext(t) want := Manifest{ - "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, } body, _ := json.Marshal(want) @@ -252,12 +206,12 @@ func TestFetchManifest_CacheHit(t *testing.T) { // First call: populates cache. result1, err := FetchManifest(ctx) require.NoError(t, err) - assert.Equal(t, "0.99.0", result1["next"].AppKit) + assert.Equal(t, "0.99.0", result1["0.296.0"].AppKit) // Second call: should come from cache, not hitting the server again. result2, err := FetchManifest(ctx) require.NoError(t, err) - assert.Equal(t, "0.99.0", result2["next"].AppKit) + assert.Equal(t, "0.99.0", result2["0.296.0"].AppKit) assert.Equal(t, int32(1), callCount.Load(), "server should only be called once; second call should be a cache hit") } @@ -265,7 +219,6 @@ func TestFetchManifest_CacheHit(t *testing.T) { func TestFetchManifest_RetryOnTransientError(t *testing.T) { ctx := testContext(t) want := Manifest{ - "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, } body, _ := json.Marshal(want) @@ -284,17 +237,16 @@ func TestFetchManifest_RetryOnTransientError(t *testing.T) { result, err := FetchManifest(ctx) require.NoError(t, err) - assert.Equal(t, "0.99.0", result["next"].AppKit) + assert.Equal(t, "0.99.0", result["0.296.0"].AppKit) assert.Equal(t, int32(2), callCount.Load(), "should have retried after first failure") } // --- parseManifest tests --- func TestParseManifest_Valid(t *testing.T) { - data := `{"next":{"appkit":"0.27.0","skills":"0.1.5"},"0.296.0":{"appkit":"0.27.0","skills":"0.1.5"}}` + data := `{"0.296.0":{"appkit":"0.27.0","skills":"0.1.5"}}` m, err := parseManifest([]byte(data)) require.NoError(t, err) - assert.Equal(t, "0.27.0", m["next"].AppKit) assert.Equal(t, "0.27.0", m["0.296.0"].AppKit) } @@ -304,13 +256,6 @@ func TestParseManifest_InvalidJSON(t *testing.T) { assert.Contains(t, err.Error(), "invalid manifest JSON") } -func TestParseManifest_MissingNext(t *testing.T) { - data := `{"0.296.0":{"appkit":"0.27.0","skills":"0.1.5"}}` - _, err := parseManifest([]byte(data)) - assert.Error(t, err) - assert.Contains(t, err.Error(), `missing "next" key`) -} - func TestParseManifest_Empty(t *testing.T) { _, err := parseManifest([]byte("{}")) assert.Error(t, err) @@ -322,7 +267,6 @@ func TestParseManifest_Empty(t *testing.T) { func TestResolveEntry_RemoteSuccess(t *testing.T) { ctx := testContext(t) want := Manifest{ - "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, } body, _ := json.Marshal(want) @@ -367,47 +311,26 @@ func TestResolveEmbeddedAppKitVersion(t *testing.T) { func TestEmbeddedManifest_IsWellFormed(t *testing.T) { m, err := parseManifest(build.CLICompatManifestJSON) require.NoError(t, err, "embedded manifest must be valid JSON") + require.NotEmpty(t, m, "embedded manifest must have at least one entry") - // Must have "next" key. - next, ok := m[nextKey] - require.True(t, ok, "embedded manifest must have %q key", nextKey) - assert.NotEmpty(t, next.AppKit, "next.appkit must be set") - assert.NotEmpty(t, next.AgentSkills, "next.skills must be set") - - // Must have at least one versioned entry. - var versionedKeys []string + // All keys must be valid semver. + var keys []string for k := range m { - if k != nextKey { - versionedKeys = append(versionedKeys, k) - } - } - assert.NotEmpty(t, versionedKeys, "must have at least one versioned CLI entry besides %q", nextKey) - - // All versioned keys must be valid semver. - for _, k := range versionedKeys { assert.True(t, semver.IsValid("v"+k), "key %q must be valid semver", k) + keys = append(keys, k) } // Sort to get deterministic order from map iteration. - slices.SortFunc(versionedKeys, func(a, b string) int { + slices.SortFunc(keys, func(a, b string) int { return semver.Compare("v"+a, "v"+b) }) - // "next" versions must be >= all versioned entries. - for _, k := range versionedKeys { - entry := m[k] - assert.GreaterOrEqual(t, semver.Compare("v"+next.AppKit, "v"+entry.AppKit), 0, - "next.appkit (%s) must be >= %s.appkit (%s)", next.AppKit, k, entry.AppKit) - assert.GreaterOrEqual(t, semver.Compare("v"+next.AgentSkills, "v"+entry.AgentSkills), 0, - "next.skills (%s) must be >= %s.skills (%s)", next.AgentSkills, k, entry.AgentSkills) - } - - // Versioned keys must be in ascending semver order. - for i := 1; i < len(versionedKeys); i++ { - cmp := semver.Compare("v"+versionedKeys[i-1], "v"+versionedKeys[i]) + // Keys must be in ascending semver order. + for i := 1; i < len(keys); i++ { + cmp := semver.Compare("v"+keys[i-1], "v"+keys[i]) assert.LessOrEqual(t, cmp, 0, - "versioned keys must be in ascending order: %s should come before %s", - versionedKeys[i-1], versionedKeys[i]) + "keys must be in ascending order: %s should come before %s", + keys[i-1], keys[i]) } } @@ -422,7 +345,6 @@ func TestManifestLocalPath(t *testing.T) { func TestReadWriteLocalManifest(t *testing.T) { ctx := testContext(t) m := Manifest{ - "next": {AppKit: "0.50.0", AgentSkills: "0.5.0"}, "0.300.0": {AppKit: "0.50.0", AgentSkills: "0.5.0"}, } @@ -431,7 +353,7 @@ func TestReadWriteLocalManifest(t *testing.T) { cached, err := readLocalManifest(path) require.NoError(t, err) - assert.Equal(t, "0.50.0", cached.manifest["next"].AppKit) + assert.Equal(t, "0.50.0", cached.manifest["0.300.0"].AppKit) assert.True(t, cached.isFresh(cacheTTL), "just-written file should be fresh") } @@ -463,33 +385,40 @@ func TestIsNotFoundError(t *testing.T) { // --- parseManifest entry validation tests --- func TestParseManifest_EmptyAppKit(t *testing.T) { - data := `{"next":{"appkit":"","skills":"0.1.5"}}` + data := `{"0.296.0":{"appkit":"","skills":"0.1.5"}}` _, err := parseManifest([]byte(data)) require.Error(t, err) assert.Contains(t, err.Error(), "empty appkit version") } func TestParseManifest_EmptySkills(t *testing.T) { - data := `{"next":{"appkit":"0.27.0","skills":""}}` + data := `{"0.296.0":{"appkit":"0.27.0","skills":""}}` _, err := parseManifest([]byte(data)) require.Error(t, err) assert.Contains(t, err.Error(), "empty skills version") } func TestParseManifest_InvalidAppKitSemver(t *testing.T) { - data := `{"next":{"appkit":"not-semver","skills":"0.1.5"}}` + data := `{"0.296.0":{"appkit":"not-semver","skills":"0.1.5"}}` _, err := parseManifest([]byte(data)) require.Error(t, err) assert.Contains(t, err.Error(), "invalid appkit version") } func TestParseManifest_InvalidSkillsSemver(t *testing.T) { - data := `{"next":{"appkit":"0.27.0","skills":"abc"}}` + data := `{"0.296.0":{"appkit":"0.27.0","skills":"abc"}}` _, err := parseManifest([]byte(data)) require.Error(t, err) assert.Contains(t, err.Error(), "invalid skills version") } +func TestParseManifest_InvalidKey(t *testing.T) { + data := `{"not-semver":{"appkit":"0.27.0","skills":"0.1.5"}}` + _, err := parseManifest([]byte(data)) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid semver key") +} + // --- FetchManifest no-retry-on-404 test --- func TestFetchManifest_NoRetryOn404(t *testing.T) { @@ -507,7 +436,7 @@ func TestFetchManifest_NoRetryOn404(t *testing.T) { require.NoError(t, err) embedded, _ := parseEmbeddedManifest() - assert.Equal(t, embedded["next"].AppKit, result["next"].AppKit) + assert.Equal(t, embedded["0.300.0"].AppKit, result["0.300.0"].AppKit) assert.Equal(t, int32(1), callCount.Load(), "404 should not be retried") } @@ -518,7 +447,6 @@ func TestFetchManifest_CacheDisabled(t *testing.T) { ctx = env.Set(ctx, "DATABRICKS_CACHE_ENABLED", "false") want := Manifest{ - "next": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, "0.296.0": {AppKit: "0.99.0", AgentSkills: "0.9.9"}, } body, _ := json.Marshal(want) @@ -534,12 +462,12 @@ func TestFetchManifest_CacheDisabled(t *testing.T) { // First call fetches from remote. result1, err := FetchManifest(ctx) require.NoError(t, err) - assert.Equal(t, "0.99.0", result1["next"].AppKit) + assert.Equal(t, "0.99.0", result1["0.296.0"].AppKit) // Second call should also fetch from remote (cache is disabled). result2, err := FetchManifest(ctx) require.NoError(t, err) - assert.Equal(t, "0.99.0", result2["next"].AppKit) + assert.Equal(t, "0.99.0", result2["0.296.0"].AppKit) assert.Equal(t, int32(2), callCount.Load(), "with cache disabled, both calls should hit the server") @@ -548,3 +476,23 @@ func TestFetchManifest_CacheDisabled(t *testing.T) { _, statErr := os.Stat(localPath) assert.ErrorIs(t, statErr, fs.ErrNotExist, "cache file should not exist when cache is disabled") } + +func TestFetchManifest_ForceEmbedded(t *testing.T) { + ctx := testContext(t) + ctx = env.Set(ctx, "DATABRICKS_FORCE_EMBEDDED_COMPAT", "true") + + var called bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + redirectToServer(t, srv) + + result, err := FetchManifest(ctx) + require.NoError(t, err) + assert.False(t, called, "server should not be called when DATABRICKS_FORCE_EMBEDDED_COMPAT=true") + + embedded, _ := parseManifest(build.CLICompatManifestJSON) + assert.Equal(t, embedded["0.300.0"].AppKit, result["0.300.0"].AppKit) +}