diff --git a/.agent/skills/bump-cli-compat/SKILL.md b/.agent/skills/bump-cli-compat/SKILL.md new file mode 100644 index 00000000000..a1fe768776e --- /dev/null +++ b/.agent/skills/bump-cli-compat/SKILL.md @@ -0,0 +1,132 @@ +--- +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 named flags: + +- `--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. + +## Workflow + +### Step 1: Resolve versions + +If both `--appkit` and `--skills` versions were provided, skip to Step 2. + +For any missing version, fetch the latest tag 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. For AppKit, also validate the `template-v` tag (used by `apps init`): + +```bash +# 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 any 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/clicompat/... -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 "Bump cli-compat to appkit {appkit_version}, skills {skills_version}" + +# Push and create PR +git push -u origin HEAD +gh pr create \ + --title "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/clicompat/... -run TestEmbeddedManifest` passes +EOF +)" +``` + +Show the PR URL to the user when done. + +## Examples + +### Example: With explicit versions +``` +/bump-cli-compat --appkit 0.28.0 --skills 0.1.6 +``` +Validates tags exist (including `template-v0.28.0`), updates manifest, creates PR. + +### Example: Auto-detect latest +``` +/bump-cli-compat +``` +Fetches latest tags, asks for eval confirmation, then updates and creates PR. + +### Example: Only bump AppKit +``` +/bump-cli-compat --appkit 0.28.0 +``` +Auto-detects latest skills version, asks for confirmation, then updates both. diff --git a/.github/OWNERS b/.github/OWNERS index 7cae525465a..579fcc2d44b 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/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 2f1da99bb71..d7844cce101 100644 --- a/cmd/apps/init.go +++ b/cmd/apps/init.go @@ -23,6 +23,7 @@ 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/env" @@ -38,7 +39,6 @@ const ( appkitTemplateDir = "template" appkitDefaultBranch = "main" appkitTemplateTagPfx = "template-v" - appkitDefaultVersion = "template-v0.24.0" defaultProfile = "DEFAULT" ) @@ -169,7 +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)") - cmd.Flags().StringVar(&version, "version", "", fmt.Sprintf("AppKit version to use (default: %s, use 'latest' for main branch)", appkitDefaultVersion)) + 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") @@ -805,8 +805,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 := clicompat.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 } @@ -859,6 +863,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/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..b79e04c6ffd 100644 --- a/cmd/apps/manifest.go +++ b/cmd/apps/manifest.go @@ -9,7 +9,9 @@ import ( "path/filepath" "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" ) @@ -27,7 +29,11 @@ func runManifestOnly(ctx context.Context, templatePath, branch, version string) case version != "": gitRef = normalizeVersion(version) default: - gitRef = appkitDefaultVersion + 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) + } + gitRef = normalizeVersion(appkitVersion) } templateSrc = appkitRepoURL } @@ -39,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 1be1538c9a0..56d323a1423 100644 --- a/experimental/aitools/cmd/list.go +++ b/experimental/aitools/cmd/list.go @@ -48,10 +48,13 @@ 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) + 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/cmd/version.go b/experimental/aitools/cmd/version.go index 67c38fec42a..a5bde04e95d 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:") @@ -80,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/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..5bdeb871bb3 100644 --- a/experimental/aitools/lib/installer/installer.go +++ b/experimental/aitools/lib/installer/installer.go @@ -16,6 +16,7 @@ import ( "github.com/databricks/cli/experimental/aitools/lib/agents" "github.com/databricks/cli/internal/build" + "github.com/databricks/cli/libs/clicompat" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" @@ -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 := clicompat.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. @@ -88,12 +93,34 @@ func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byt return io.ReadAll(resp.Body) } +// 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") + manifest, err := src.FetchManifest(ctx, ref) + if err != nil && clicompat.IsNotFoundError(err) { + fallbackVersion, fbErr := clicompat.ResolveEmbeddedAgentSkillsVersion() + if fbErr == nil && fallbackVersion != "" && fallbackVersion != tag { + log.Warnf(ctx, "Skills version %s not found, falling back to embedded version %s", tag, fallbackVersion) + ref = "v" + 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 := GetSkillsRef(ctx) - manifest, err := src.FetchManifest(ctx, ref) + 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 } @@ -193,12 +220,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..848ab54172c 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)) @@ -725,3 +742,19 @@ func TestSupportsProjectScopeSetCorrectly(t *testing.T) { assert.Equal(t, want, agent.SupportsProjectScope, "SupportsProjectScope for %s", agent.Name) } } + +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/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/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..ea3674116be 100644 --- a/experimental/aitools/lib/installer/update.go +++ b/experimental/aitools/lib/installer/update.go @@ -81,14 +81,17 @@ 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.") 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/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..82e3b5df9dd --- /dev/null +++ b/internal/build/README.md @@ -0,0 +1,63 @@ +# 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.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 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 + +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 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. +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 or Agent Skills release: + +1. **Run evals** on the new AppKit version. If there is no regression, proceed. +2. **Open a PR** to update `cli-compat.json`. The change depends on the type of release: + - **No template changes** (just an AppKit/skills version bump): search & replace all version occurrences in the manifest and update `next`. + - **Template changes that don't require new CLI features**: test the last 3 CLI versions with the new template and update matching entries. + - **Template changes that require new CLI features**: add a new entry for the minimum CLI version that supports them; older entries keep pointing to the previous template version. + +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/clicompat/`: + +```bash +go test ./libs/clicompat/... -run TestEmbeddedManifest -v +``` + +This checks: valid JSON, `"next"` key present, at least one versioned entry, valid semver keys, valid semver entry values, `next` versions >= all entries, and ascending key order. + +## 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/internal/build/cli-compat.json b/internal/build/cli-compat.json new file mode 100644 index 00000000000..33fc41f0a64 --- /dev/null +++ b/internal/build/cli-compat.json @@ -0,0 +1,4 @@ +{ + "next": { "appkit": "0.24.0", "skills": "0.1.5" }, + "0.300.0": { "appkit": "0.24.0", "skills": "0.1.5" } +} diff --git a/internal/build/clicompat.go b/internal/build/clicompat.go new file mode 100644 index 00000000000..22cb8f13a03 --- /dev/null +++ b/internal/build/clicompat.go @@ -0,0 +1,9 @@ +package build + +import _ "embed" + +// 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 CLICompatManifestJSON []byte diff --git a/libs/clicompat/clicompat.go b/libs/clicompat/clicompat.go new file mode 100644 index 00000000000..0ffe76e4827 --- /dev/null +++ b/libs/clicompat/clicompat.go @@ -0,0 +1,444 @@ +package clicompat + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "slices" + "strings" + "sync" + "time" + + "github.com/databricks/cli/internal/build" + "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/log" + "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" + + // fetchTimeout is the HTTP timeout for fetching the manifest at runtime. + fetchTimeout = 3 * time.Second + + // nextKey is the special manifest key for CLI versions newer than any entry. + nextKey = "next" + + // cacheTTL is how long a locally cached manifest is considered fresh. + cacheTTL = 1 * time.Hour + + // localManifestFile is the filename for the locally cached manifest. + localManifestFile = "compat-manifest.json" + + // devVersionPrefix identifies dev builds that always use the "next" entry. + devVersionPrefix = "0.0.0-dev" + + maxFetchAttempts = 3 + 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. 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: +// 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 +// +// 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. + 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) { + 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 { + if cacheEnabled { + 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. + m, embeddedErr := parseEmbeddedManifest() + 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: %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 := parseEmbeddedManifest() + 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) + } + 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 := parseEmbeddedManifest() + 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) + } + 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 + } + 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") +} + +// 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, devVersionPrefix) { + 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 { + log.Debugf(ctx, "Could not determine user cache directory: %v", err) + 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 writes the manifest to the local cache path using a +// temp-file-then-rename pattern for atomicity. +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.Warnf(ctx, "Failed to create cache directory %s: %v", dir, err) + return + } + tmp, err := os.CreateTemp(dir, ".compat-manifest-*.tmp") + if err != nil { + log.Warnf(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 + } + if err := os.Rename(tmpPath, path); err != nil { + log.Warnf(ctx, "Failed to rename temp cache file: %v", err) + } +} + +// --- Remote fetch --- + +// 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 maxFetchAttempts { + if attempt > 0 { + log.Debugf(ctx, "Retrying manifest fetch (attempt %d/%d)", attempt+1, maxFetchAttempts) + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(fetchRetryBackoff): + } + } + m, err := fetchRemote(ctx) + if err == nil { + 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 +} + +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 + } + req.Header.Set("User-Agent", "databricks-cli/"+build.GetInfo().Version) + + resp, err := httpClient.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, &HTTPStatusError{StatusCode: 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) + } + } + 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 new file mode 100644 index 00000000000..a2351254c60 --- /dev/null +++ b/libs/clicompat/clicompat_test.go @@ -0,0 +1,550 @@ +package clicompat + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io/fs" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "slices" + "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.CLICompatManifestJSON) + 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) +} + +// --- ResolveEmbeddedAppKitVersion --- + +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) --- + +func TestEmbeddedManifest_IsWellFormed(t *testing.T) { + m, err := parseManifest(build.CLICompatManifestJSON) + 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) + } + + // 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] + 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") +} + +// --- 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") +}