From f2c3c3cd0d504bd596c5e782a861f1604f8371ac Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 14:59:29 +0200 Subject: [PATCH 1/7] 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 00000000000..3256b3f74b4 --- /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 7cae525465a..91084750e80 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 2f1da99bb71..5daba0e202f 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 81e71eed3fb..fe406dfb7b3 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 38df201acc0..ff200e6998f 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 1be1538c9a0..8f665d7ad12 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 67c38fec42a..17faa468a68 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 027a383a35e..00000000000 --- 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 53285f6ffc9..39e04120ba7 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 b769143906d..df3bf8f6b0a 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 6c7589f6f29..444fb773af7 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 663ad5e908e..0aa260f115a 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 97e3014be65..3272a77babe 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 0e942ca0a95..00000000000 --- 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 00000000000..71409737c72 --- /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 00000000000..b7cff3b8312 --- /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 00000000000..c5bc84635cb --- /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 00000000000..da2febc59e2 --- /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 00000000000..73be235ad63 --- /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 c73fe7be4b5cc67368f4a696c63c206097110a49 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 15:16:37 +0200 Subject: [PATCH 2/7] 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 17faa468a68..a5bde04e95d 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 df3bf8f6b0a..36ab7d9dc94 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 da2febc59e2..2a08e427a09 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 73be235ad63..876e06ce167 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 6c1952ed58ac8f45da3c1286d7877478d908868c Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 15:23:25 +0200 Subject: [PATCH 3/7] 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 3256b3f74b4..a1fe768776e 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 91084750e80..579fcc2d44b 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 5daba0e202f..6cd3f5e82c8 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 ff200e6998f..96b000aa18e 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 39e04120ba7..60c7e16080e 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 71409737c72..d162fe2b3f7 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 c5bc84635cb..22cb8f13a03 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 2a08e427a09..5f7941f8711 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 876e06ce167..73459e32909 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 06b1733b8241406db4da471c0eeb8b0e2dd4e7c5 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 15:29:51 +0200 Subject: [PATCH 4/7] 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 b7cff3b8312..33fc41f0a64 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 e88c0aa3fdc4039499c37f51ef39378e767feea1 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Tue, 5 May 2026 15:58:16 +0200 Subject: [PATCH 5/7] 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 6cd3f5e82c8..01bfa0fad69 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 96b000aa18e..da904b14715 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 60c7e16080e..c8e3c7698bc 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 36ab7d9dc94..848ab54172c 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 d162fe2b3f7..48ddcad1523 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 5f7941f8711..a9b476a8335 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 73459e32909..7fc9aeabc69 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 ea5fde1879f5194c7d674b905af0e9f51e590dc3 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Wed, 6 May 2026 10:03:01 +0200 Subject: [PATCH 6/7] 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 01bfa0fad69..35eb45cd99e 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 c8e3c7698bc..1f081f69fc6 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 a9b476a8335..e77ec7da9c2 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 7fc9aeabc69..f40f64dd806 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 702d30e1cb670ad8f72bca905aab6c132092baa9 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Wed, 6 May 2026 15:38:24 +0200 Subject: [PATCH 7/7] 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 35eb45cd99e..d7844cce101 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 da904b14715..b79e04c6ffd 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 8f665d7ad12..56d323a1423 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 1f081f69fc6..5bdeb871bb3 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 e601b26d66d..5dc78a254fd 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 0aa260f115a..ea3674116be 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 48ddcad1523..82e3b5df9dd 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 e77ec7da9c2..0ffe76e4827 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 f40f64dd806..a2351254c60 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") +}