Skip to content

Commit 1491783

Browse files
committed
fix: skip skills fallback when DATABRICKS_SKILLS_REF is explicitly set
GetSkillsRef now returns whether the ref was explicitly set via env var. FetchSkillsManifestWithFallback accepts allowFallback to skip the embedded fallback when the user explicitly chose a ref. Co-authored-by: Isaac
1 parent 847438f commit 1491783

5 files changed

Lines changed: 17 additions & 15 deletions

File tree

experimental/aitools/cmd/list.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,13 @@ func newListCmd() *cobra.Command {
4848
func defaultListSkills(cmd *cobra.Command, scope string) error {
4949
ctx := cmd.Context()
5050

51-
ref, err := installer.GetSkillsRef(ctx)
51+
ref, explicit, err := installer.GetSkillsRef(ctx)
5252
if err != nil {
5353
return err
5454
}
5555

5656
src := &installer.GitHubManifestSource{}
57-
manifest, ref, err := installer.FetchSkillsManifestWithFallback(ctx, src, ref)
57+
manifest, ref, err := installer.FetchSkillsManifestWithFallback(ctx, src, ref, !explicit)
5858
if err != nil {
5959
return fmt.Errorf("failed to fetch manifest: %w", err)
6060
}

experimental/aitools/cmd/version.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func newVersionCmd() *cobra.Command {
4545
return nil
4646
}
4747

48-
latestRef, err := installer.GetSkillsRef(ctx)
48+
latestRef, _, err := installer.GetSkillsRef(ctx)
4949
if err != nil {
5050
log.Debugf(ctx, "could not resolve skills version: %v", err)
5151
}

experimental/aitools/lib/installer/installer.go

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,18 @@ const (
3333
// It is a package-level var so tests can replace it with a mock.
3434
var fetchFileFn = fetchSkillFile
3535

36-
// GetSkillsRef returns the skills repo ref to use.
36+
// GetSkillsRef returns the skills repo ref to use and whether it was explicitly
37+
// set via DATABRICKS_SKILLS_REF (as opposed to auto-resolved from the manifest).
3738
// Resolution order: DATABRICKS_SKILLS_REF env var → compatibility manifest → error.
38-
func GetSkillsRef(ctx context.Context) (string, error) {
39+
func GetSkillsRef(ctx context.Context) (ref string, explicit bool, err error) {
3940
if ref := env.Get(ctx, "DATABRICKS_SKILLS_REF"); ref != "" {
40-
return ref, nil
41+
return ref, true, nil
4142
}
4243
v, err := clicompat.ResolveAgentSkillsVersion(ctx)
4344
if err != nil {
44-
return "", fmt.Errorf("could not resolve skills version: %w", err)
45+
return "", false, fmt.Errorf("could not resolve skills version: %w", err)
4546
}
46-
return "v" + v, nil
47+
return "v" + v, false, nil
4748
}
4849

4950
// Manifest describes the skills manifest fetched from the skills repo.
@@ -97,10 +98,10 @@ func fetchSkillFile(ctx context.Context, ref, skillName, filePath string) ([]byt
9798
// If the ref points to a non-existent tag (not-found error), it falls back to
9899
// the embedded manifest's skills version. Returns the manifest, the (possibly
99100
// updated) ref, and any error.
100-
func FetchSkillsManifestWithFallback(ctx context.Context, src ManifestSource, ref string) (*Manifest, string, error) {
101+
func FetchSkillsManifestWithFallback(ctx context.Context, src ManifestSource, ref string, allowFallback bool) (*Manifest, string, error) {
101102
tag := strings.TrimPrefix(ref, "v")
102103
manifest, err := src.FetchManifest(ctx, ref)
103-
if err != nil && clicompat.IsNotFoundError(err) {
104+
if err != nil && allowFallback && clicompat.IsNotFoundError(err) {
104105
fallbackVersion, fbErr := clicompat.ResolveEmbeddedAgentSkillsVersion()
105106
if fbErr == nil && fallbackVersion != "" && fallbackVersion != tag {
106107
log.Warnf(ctx, "Skills version %s not found, falling back to embedded version %s", tag, fallbackVersion)
@@ -117,12 +118,12 @@ func FetchSkillsManifestWithFallback(ctx context.Context, src ManifestSource, re
117118
// This is the core installation function. Callers are responsible for agent detection,
118119
// prompting, and printing the "Installing..." header.
119120
func InstallSkillsForAgents(ctx context.Context, src ManifestSource, targetAgents []*agents.Agent, opts InstallOptions) error {
120-
ref, err := GetSkillsRef(ctx)
121+
ref, explicit, err := GetSkillsRef(ctx)
121122
if err != nil {
122123
return err
123124
}
124125
cmdio.LogString(ctx, "Using skills version "+strings.TrimPrefix(ref, "v"))
125-
manifest, ref, err := FetchSkillsManifestWithFallback(ctx, src, ref)
126+
manifest, ref, err := FetchSkillsManifestWithFallback(ctx, src, ref, !explicit)
126127
if err != nil {
127128
return err
128129
}

experimental/aitools/lib/installer/installer_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -753,8 +753,9 @@ func TestGetSkillsRefResolvesFromManifest(t *testing.T) {
753753
manifest := `{"next":{"appkit":"0.24.0","skills":"0.1.5"},"0.300.0":{"appkit":"0.24.0","skills":"0.1.5"}}`
754754
require.NoError(t, os.WriteFile(cachePath, []byte(manifest), 0o644))
755755

756-
ref, err := GetSkillsRef(t.Context())
756+
ref, explicit, err := GetSkillsRef(t.Context())
757757
require.NoError(t, err, "GetSkillsRef should succeed via cached manifest")
758+
assert.False(t, explicit, "ref resolved from manifest should not be explicit")
758759
assert.NotEmpty(t, ref)
759760
assert.True(t, len(ref) > 1 && ref[0] == 'v', "ref should start with 'v', got %q", ref)
760761
}

experimental/aitools/lib/installer/update.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent
8181
return nil, errors.New("no skills installed. Run 'databricks experimental aitools install' to install")
8282
}
8383

84-
latestTag, err := GetSkillsRef(ctx)
84+
latestTag, explicit, err := GetSkillsRef(ctx)
8585
if err != nil {
8686
return nil, err
8787
}
@@ -91,7 +91,7 @@ func UpdateSkills(ctx context.Context, src ManifestSource, targetAgents []*agent
9191
return &UpdateResult{Unchanged: slices.Sorted(maps.Keys(state.Skills))}, nil
9292
}
9393

94-
manifest, latestTag, err := FetchSkillsManifestWithFallback(ctx, src, latestTag)
94+
manifest, latestTag, err := FetchSkillsManifestWithFallback(ctx, src, latestTag, !explicit)
9595
if err != nil {
9696
if opts.Check {
9797
log.Warnf(ctx, "Could not fetch manifest: %v", err)

0 commit comments

Comments
 (0)