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

Author: pkosiec

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 <plugin>.sql-warehouse.id=<value> instead")

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
 	}

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)
 	}

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
 	}

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)
 	}

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)

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.

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,19 +78,34 @@ 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:
 //  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.
-	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
 }