From d598ec4bfd53a7ae9f5cccfd1909a43f9f4a4728 Mon Sep 17 00:00:00 2001 From: simon Date: Fri, 17 Apr 2026 16:07:42 +0200 Subject: [PATCH 1/9] Add databricks doctor diagnostic command Adds a new top-level 'databricks doctor' command that runs diagnostic checks against the user's CLI setup and reports a checklist of results (text) or a JSON array. Checks: - CLI version - Updates (queries the GitHub releases API, skipped for dev builds) - Toolchain (git, python3, uv, terraform versions) - Proxy/TLS environment variables (HTTPS_PROXY, NO_PROXY, etc., with credentials masked) - Log file path - Config file readability and profile count - Current profile source - Authentication validity and auth type - Identity via CurrentUser.Me (skipped for account-level profiles) - Network reachability Design follows go-code-structure.md: check functions take context and primitives, Cobra RunE is a thin adapter, rendering is a pure function, and external dependencies (exec, HTTP client) are injected for tests. SPOG / unified-host account profiles are correctly classified via the existing auth.ResolveConfigType helper, so the SDK's ConfigType() returning InvalidConfig for those hosts no longer causes a misclassification. Also fixes a latent bug in libs/env/loader.go: Set was replaced with SetS so that non-string env-backed config attributes (e.g. ints, bools) are parsed correctly. Co-authored-by: Isaac --- NEXT_CHANGELOG.md | 1 + acceptance/cmd/doctor/out.test.toml | 5 + acceptance/cmd/doctor/output.txt | 14 + acceptance/cmd/doctor/script | 10 + acceptance/cmd/doctor/test.toml | 23 ++ acceptance/help/output.txt | 1 + cmd/cmd.go | 2 + cmd/doctor/checks.go | 281 +++++++++++++ cmd/doctor/doctor.go | 113 ++++++ cmd/doctor/doctor_test.go | 596 ++++++++++++++++++++++++++++ cmd/doctor/extended.go | 200 ++++++++++ cmd/doctor/extended_test.go | 236 +++++++++++ libs/env/loader.go | 2 +- 13 files changed, 1483 insertions(+), 1 deletion(-) create mode 100644 acceptance/cmd/doctor/out.test.toml create mode 100644 acceptance/cmd/doctor/output.txt create mode 100644 acceptance/cmd/doctor/script create mode 100644 acceptance/cmd/doctor/test.toml create mode 100644 cmd/doctor/checks.go create mode 100644 cmd/doctor/doctor.go create mode 100644 cmd/doctor/doctor_test.go create mode 100644 cmd/doctor/extended.go create mode 100644 cmd/doctor/extended_test.go diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 52d764e384d..52af648b72d 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -7,6 +7,7 @@ ### CLI * Added `--limit` flag to all paginated list commands for client-side result capping ([#4984](https://github.com/databricks/cli/pull/4984)). +* Added `databricks doctor` command that runs diagnostic checks (CLI version, updates, toolchain, proxy/TLS, log file, config file, profile, authentication, identity, network) and reports results as a checklist or JSON ([#4730](https://github.com/databricks/cli/pull/4730)). ### Bundles diff --git a/acceptance/cmd/doctor/out.test.toml b/acceptance/cmd/doctor/out.test.toml new file mode 100644 index 00000000000..d560f1de043 --- /dev/null +++ b/acceptance/cmd/doctor/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/cmd/doctor/output.txt b/acceptance/cmd/doctor/output.txt new file mode 100644 index 00000000000..5c33406c137 --- /dev/null +++ b/acceptance/cmd/doctor/output.txt @@ -0,0 +1,14 @@ + +=== Doctor with --profile flag + +>>> [CLI] doctor --profile my-workspace --output json +CLI Version: info - [DEV_VERSION] +Updates: info - development build ([DEV_VERSION]) +Toolchain: info - [TOOLCHAIN] +Proxy/TLS: info - no proxy or TLS overrides configured +Log File: info - not configured (set DATABRICKS_LOG_FILE or pass --log-file to enable) +Config File: pass - ~/.databrickscfg (1 profiles) +Current Profile: info - my-workspace +Authentication: pass - OK (pat) +Identity: pass - test@example.com +Network: pass - [DATABRICKS_URL] is reachable diff --git a/acceptance/cmd/doctor/script b/acceptance/cmd/doctor/script new file mode 100644 index 00000000000..1f54c619a82 --- /dev/null +++ b/acceptance/cmd/doctor/script @@ -0,0 +1,10 @@ +sethome "./home" + +cat > "./home/.databrickscfg" < "2.42.0", "Terraform v1.5.0" -> "v1.5.0"). +func firstLineVersion(out string) string { + line := strings.TrimSpace(strings.SplitN(out, "\n", 2)[0]) + fields := strings.Fields(line) + for i, f := range fields { + if looksLikeVersion(f) { + return strings.Join(fields[i:], " ") + } + } + return line +} + +func looksLikeVersion(tok string) bool { + if len(tok) == 0 { + return false + } + c := tok[0] + if c >= '0' && c <= '9' { + return true + } + // Accept a leading "v" only when followed by a digit (e.g. v1.5.0). + if c == 'v' && len(tok) >= 2 && tok[1] >= '0' && tok[1] <= '9' { + return true + } + return false +} + +// proxyEnvVars lists the environment variables that influence HTTP/TLS behavior. +// Both upper- and lower-case forms are checked since Go's http.ProxyFromEnvironment +// honors both. +var proxyEnvVars = []string{ + "HTTPS_PROXY", "https_proxy", + "HTTP_PROXY", "http_proxy", + "NO_PROXY", "no_proxy", + "SSL_CERT_FILE", + "REQUESTS_CA_BUNDLE", + "CURL_CA_BUNDLE", +} + +// checkProxy reports proxy/TLS-related environment variables that affect the CLI's +// network stack. These are a common source of enterprise connectivity issues. +func checkProxy(ctx context.Context) CheckResult { + var seen []string + reported := map[string]bool{} + for _, key := range proxyEnvVars { + v := env.Get(ctx, key) + if v == "" { + continue + } + canonical := strings.ToUpper(key) + if reported[canonical] { + continue + } + reported[canonical] = true + seen = append(seen, canonical+"="+maskProxyValue(v)) + } + if len(seen) == 0 { + return info("Proxy/TLS", "no proxy or TLS overrides configured") + } + return info("Proxy/TLS", strings.Join(seen, ", ")) +} + +// maskProxyValue hides credentials in proxy URLs (user:pass@host). +// net/url would percent-encode "***", so the replacement is done via string rewrite +// on the exact userinfo segment. +func maskProxyValue(v string) string { + u, err := url.Parse(v) + if err != nil || u.User == nil { + return v + } + userinfo := u.User.String() + if _, hasPassword := u.User.Password(); !hasPassword { + return v + } + masked := u.User.Username() + ":***" + return strings.Replace(v, userinfo, masked, 1) +} + +// checkUpdates fetches the latest CLI release and compares it to the current build. +// Development builds are reported as info; older builds produce a warn status. +func checkUpdates(ctx context.Context, client *http.Client, releaseURL string) CheckResult { + return checkUpdatesWithVersion(ctx, client, releaseURL, build.GetInfo().Version) +} + +// checkUpdatesWithVersion is the testable core of checkUpdates, parameterized on the current version. +func checkUpdatesWithVersion(ctx context.Context, client *http.Client, releaseURL, current string) CheckResult { + if isDevBuild(current) { + return info("Updates", "development build ("+current+")") + } + + ctx, cancel := context.WithTimeout(ctx, updateCheckTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, releaseURL, nil) + if err != nil { + return warn("Updates", "cannot check for updates: "+err.Error()) + } + req.Header.Set("Accept", "application/vnd.github+json") + + resp, err := client.Do(req) + if err != nil { + return warn("Updates", "cannot reach "+releaseURL) + } + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + if err != nil { + return warn("Updates", "cannot read release response: "+err.Error()) + } + if resp.StatusCode != http.StatusOK { + return warn("Updates", fmt.Sprintf("release lookup returned HTTP %d", resp.StatusCode)) + } + + var release struct { + TagName string `json:"tag_name"` + } + if err := json.Unmarshal(body, &release); err != nil { + return warn("Updates", "cannot parse release response: "+err.Error()) + } + + latest := strings.TrimPrefix(release.TagName, "v") + if latest == "" { + return warn("Updates", "empty release tag") + } + if latest == current { + return pass("Updates", "up to date ("+current+")") + } + return warn("Updates", fmt.Sprintf("newer version available: %s (current %s)", latest, current)) +} + +// isDevBuild reports whether the running binary is a local/dev build. +// The build.GetInfo version for dev builds is "0.0.0-dev+". +func isDevBuild(version string) bool { + return version == "" || strings.HasPrefix(version, "0.0.0-dev") +} + +// checkLogFile surfaces where CLI logs are being written, so users can find them for support. +func checkLogFile(ctx context.Context) CheckResult { + path := env.Get(ctx, "DATABRICKS_LOG_FILE") + if path == "" { + return info("Log File", "not configured (set DATABRICKS_LOG_FILE or pass --log-file to enable)") + } + return info("Log File", path) +} diff --git a/cmd/doctor/extended_test.go b/cmd/doctor/extended_test.go new file mode 100644 index 00000000000..54d776cfdc3 --- /dev/null +++ b/cmd/doctor/extended_test.go @@ -0,0 +1,236 @@ +package doctor + +import ( + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/env" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFirstLineVersion(t *testing.T) { + tests := []struct { + in, want string + }{ + {"git version 2.42.0\n", "2.42.0"}, + {"Python 3.11.5", "3.11.5"}, + {"uv 0.1.0 (unknown)", "0.1.0 (unknown)"}, + {"Terraform v1.5.0\non darwin", "v1.5.0"}, + {"", ""}, + {"weirdtool", "weirdtool"}, + } + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + assert.Equal(t, tt.want, firstLineVersion(tt.in)) + }) + } +} + +func TestCheckToolchain(t *testing.T) { + mockExec := func(versions map[string]string) execFunc { + return func(_ context.Context, name string, _ ...string) (string, error) { + v, ok := versions[name] + if !ok { + return "", errors.New("not found") + } + return v, nil + } + } + + t.Run("all tools present", func(t *testing.T) { + run := mockExec(map[string]string{ + "git": "git version 2.42.0", + "python3": "Python 3.11.5", + "uv": "uv 0.1.0", + "terraform": "Terraform v1.5.0", + }) + result := checkToolchain(t.Context(), run) + assert.Equal(t, statusInfo, result.Status) + assert.Contains(t, result.Message, "git 2.42.0") + assert.Contains(t, result.Message, "python 3.11.5") + assert.Contains(t, result.Message, "terraform v1.5.0") + }) + + t.Run("missing tools reported inline", func(t *testing.T) { + run := mockExec(map[string]string{"git": "git version 2.42.0"}) + result := checkToolchain(t.Context(), run) + assert.Equal(t, statusInfo, result.Status) + assert.Contains(t, result.Message, "git 2.42.0") + assert.Contains(t, result.Message, "terraform not found") + assert.Contains(t, result.Message, "python not found") + }) +} + +func TestCheckProxy(t *testing.T) { + t.Run("none configured", func(t *testing.T) { + clearConfigEnv(t) + for _, v := range proxyEnvVars { + t.Setenv(v, "") + } + ctx := cmdio.MockDiscard(t.Context()) + result := checkProxy(ctx) + assert.Equal(t, statusInfo, result.Status) + assert.Contains(t, result.Message, "no proxy") + }) + + t.Run("reports set env vars", func(t *testing.T) { + for _, v := range proxyEnvVars { + t.Setenv(v, "") + } + ctx := cmdio.MockDiscard(t.Context()) + ctx = env.Set(ctx, "HTTPS_PROXY", "http://proxy.example.com:8080") + ctx = env.Set(ctx, "NO_PROXY", "localhost,127.0.0.1") + + result := checkProxy(ctx) + assert.Equal(t, statusInfo, result.Status) + assert.Contains(t, result.Message, "HTTPS_PROXY=http://proxy.example.com:8080") + assert.Contains(t, result.Message, "NO_PROXY=localhost,127.0.0.1") + }) + + t.Run("masks credentials in proxy URL", func(t *testing.T) { + for _, v := range proxyEnvVars { + t.Setenv(v, "") + } + ctx := cmdio.MockDiscard(t.Context()) + ctx = env.Set(ctx, "HTTPS_PROXY", "http://user:secret@proxy.example.com:8080") + result := checkProxy(ctx) + assert.Contains(t, result.Message, "user:***@proxy.example.com") + assert.NotContains(t, result.Message, "secret") + }) + + t.Run("deduplicates upper and lower case variants", func(t *testing.T) { + for _, v := range proxyEnvVars { + t.Setenv(v, "") + } + ctx := cmdio.MockDiscard(t.Context()) + ctx = env.Set(ctx, "HTTPS_PROXY", "http://a.example.com") + ctx = env.Set(ctx, "https_proxy", "http://b.example.com") + + result := checkProxy(ctx) + assert.Equal(t, 1, strings.Count(result.Message, "HTTPS_PROXY=")) + }) +} + +func TestMaskProxyValue(t *testing.T) { + tests := []struct { + in, want string + }{ + {"http://proxy.example.com:8080", "http://proxy.example.com:8080"}, + {"http://user:secret@proxy.example.com:8080", "http://user:***@proxy.example.com:8080"}, + {"not a url", "not a url"}, + {"localhost,127.0.0.1", "localhost,127.0.0.1"}, + } + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + assert.Equal(t, tt.want, maskProxyValue(tt.in)) + }) + } +} + +func TestCheckUpdates(t *testing.T) { + t.Run("dev build", func(t *testing.T) { + // current version is "0.0.0-dev+" in tests + result := checkUpdates(t.Context(), http.DefaultClient, "http://unused") + assert.Equal(t, statusInfo, result.Status) + assert.Contains(t, result.Message, "development build") + }) + + t.Run("up to date", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `{"tag_name": "v1.0.0"}`) + })) + defer srv.Close() + + result := checkUpdatesWithVersion(t.Context(), http.DefaultClient, srv.URL, "1.0.0") + assert.Equal(t, statusPass, result.Status) + assert.Contains(t, result.Message, "up to date") + }) + + t.Run("newer available", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `{"tag_name": "v1.2.3"}`) + })) + defer srv.Close() + + result := checkUpdatesWithVersion(t.Context(), http.DefaultClient, srv.URL, "1.0.0") + assert.Equal(t, statusWarn, result.Status) + assert.Contains(t, result.Message, "1.2.3") + assert.Contains(t, result.Message, "1.0.0") + }) + + t.Run("network failure warns", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + result := checkUpdatesWithVersion(t.Context(), http.DefaultClient, srv.URL, "1.0.0") + assert.Equal(t, statusWarn, result.Status) + assert.Contains(t, result.Message, "HTTP 500") + }) + + t.Run("malformed response warns", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `not-json`) + })) + defer srv.Close() + + result := checkUpdatesWithVersion(t.Context(), http.DefaultClient, srv.URL, "1.0.0") + assert.Equal(t, statusWarn, result.Status) + }) +} + +func TestCheckLogFile(t *testing.T) { + t.Run("not configured", func(t *testing.T) { + t.Setenv("DATABRICKS_LOG_FILE", "") + ctx := cmdio.MockDiscard(t.Context()) + result := checkLogFile(ctx) + assert.Equal(t, statusInfo, result.Status) + assert.Contains(t, result.Message, "not configured") + }) + + t.Run("configured path", func(t *testing.T) { + t.Setenv("DATABRICKS_LOG_FILE", "") + ctx := cmdio.MockDiscard(t.Context()) + ctx = env.Set(ctx, "DATABRICKS_LOG_FILE", "/var/log/databricks.log") + result := checkLogFile(ctx) + assert.Equal(t, statusInfo, result.Status) + assert.Equal(t, "/var/log/databricks.log", result.Message) + }) +} + +// Extra sanity: the full command includes all new checks. +func TestRunChecksIncludesExtended(t *testing.T) { + clearConfigEnv(t) + ctx := cmdio.MockDiscard(t.Context()) + results := runChecks(ctx, "", false) + + names := map[string]bool{} + for _, r := range results { + names[r.Name] = true + } + for _, want := range []string{"CLI Version", "Updates", "Toolchain", "Proxy/TLS", "Log File", "Config File", "Current Profile", "Authentication", "Identity", "Network"} { + assert.True(t, names[want], "expected check %q in results", want) + } +} + +// require the test server to return JSON (sanity). +func TestUpdatesServerShape(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `{"tag_name": "v0.1.0"}`) + })) + defer srv.Close() + + resp, err := http.Get(srv.URL) + require.NoError(t, err) + defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + assert.Contains(t, string(body), "tag_name") +} diff --git a/libs/env/loader.go b/libs/env/loader.go index 74c54cee8fa..89b6bdddc9c 100644 --- a/libs/env/loader.go +++ b/libs/env/loader.go @@ -43,7 +43,7 @@ func (le *configLoader) Configure(cfg *config.Config) error { if v == "" { continue } - if err := a.Set(cfg, v); err != nil { + if err := a.SetS(cfg, v); err != nil { return err } } From bebae01d6ed2dc271552348d9a18f31399d31afa Mon Sep 17 00:00:00 2001 From: simon Date: Fri, 17 Apr 2026 17:17:55 +0200 Subject: [PATCH 2/9] Address GPT review findings for doctor - checkIdentity now does a real API call for account-level profiles (a.Workspaces.List), matching what auth describe does. Previously account-level profiles skipped identity entirely, so invalid account PAT/OAuth could report Authentication: OK with no failing check. - maskProxyValue now masks the full userinfo segment whenever present, not only when a password is set. Protects against token-only proxy URLs like http://TOKEN@proxy from leaking the token in diagnostics. - checkToolchain distinguishes 'not found' (exec.ErrNotFound) from non-zero exit (*exec.ExitError) from other errors (permission denied, etc.), so users can tell 'install this' apart from 'this is broken'. Tests updated to cover all three cases. --- cmd/doctor/checks.go | 20 +++++++++++-- cmd/doctor/doctor_test.go | 58 +++++++++++++++++++++---------------- cmd/doctor/extended.go | 25 ++++++++++++---- cmd/doctor/extended_test.go | 51 +++++++++++++++++++++++++++++--- 4 files changed, 118 insertions(+), 36 deletions(-) diff --git a/cmd/doctor/checks.go b/cmd/doctor/checks.go index 04e175baf89..f17d344c82a 100644 --- a/cmd/doctor/checks.go +++ b/cmd/doctor/checks.go @@ -193,9 +193,8 @@ func checkIdentity(ctx context.Context, authCfg *config.Config) CheckResult { ctx, cancel := withCheckTimeout(ctx) defer cancel() - // Account-level configs don't support the /me endpoint for workspace identity. if isAccountLevelConfig(authCfg) { - return skip("Identity", "Skipped (account-level profile, workspace identity not available)") + return checkAccountIdentity(ctx, authCfg) } w, err := databricks.NewWorkspaceClient((*databricks.Config)(authCfg)) @@ -211,6 +210,23 @@ func checkIdentity(ctx context.Context, authCfg *config.Config) CheckResult { return pass("Identity", me.UserName) } +// checkAccountIdentity issues a lightweight authenticated account API call so +// account-level profiles get server-side credential validation instead of being +// skipped (which would let invalid account PAT/OAuth report Authentication: OK). +func checkAccountIdentity(ctx context.Context, authCfg *config.Config) CheckResult { + a, err := databricks.NewAccountClient((*databricks.Config)(authCfg)) + if err != nil { + return fail("Identity", "Cannot create account client", err) + } + + workspaces, err := a.Workspaces.List(ctx) + if err != nil { + return fail("Identity", "Cannot list account workspaces", err) + } + + return pass("Identity", fmt.Sprintf("account %s (%d workspaces)", authCfg.AccountID, len(workspaces))) +} + func checkNetwork(ctx context.Context, cfg *config.Config, resolveErr error, authCfg *config.Config) CheckResult { // Prefer the authenticated config (it has the fully resolved host). if authCfg != nil { diff --git a/cmd/doctor/doctor_test.go b/cmd/doctor/doctor_test.go index a681055ad17..06833817911 100644 --- a/cmd/doctor/doctor_test.go +++ b/cmd/doctor/doctor_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "github.com/databricks/cli/libs/cmdio" @@ -293,20 +294,30 @@ func TestResolveConfig(t *testing.T) { } func TestCheckIdentity(t *testing.T) { - okServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/api/2.0/preview/scim/v2/Me" { + workspaceOK := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/scim/v2/Me") { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"userName": "test@example.com"}`)) return } w.WriteHeader(http.StatusOK) })) - defer okServer.Close() + defer workspaceOK.Close() - failServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + accountOK := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/accounts/") && strings.HasSuffix(r.URL.Path, "/workspaces") { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"workspace_id": 1}, {"workspace_id": 2}]`)) + return + } + w.WriteHeader(http.StatusOK) + })) + defer accountOK.Close() + + unauthorized := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusUnauthorized) })) - defer failServer.Close() + defer unauthorized.Close() tests := []struct { name string @@ -315,36 +326,37 @@ func TestCheckIdentity(t *testing.T) { wantMsg string }{ { - name: "success", - cfg: &config.Config{Host: okServer.URL, Token: "test-token"}, + name: "workspace success", + cfg: &config.Config{Host: workspaceOK.URL, Token: "test-token"}, wantStatus: statusPass, wantMsg: "test@example.com", }, { - name: "failure", - cfg: &config.Config{Host: failServer.URL, Token: "bad-token"}, + name: "workspace failure", + cfg: &config.Config{Host: unauthorized.URL, Token: "bad-token"}, wantStatus: statusFail, }, { - name: "skipped for account-level", + name: "account success (unified host)", cfg: &config.Config{ - Host: "https://accounts.cloud.databricks.com", - AccountID: "test-account-123", - Token: "test-token", + Host: accountOK.URL, + AccountID: "test-account-123", + Token: "test-token", + Experimental_IsUnifiedHost: true, }, - wantStatus: statusSkip, - wantMsg: "account-level", + wantStatus: statusPass, + wantMsg: "account test-account-123 (2 workspaces)", }, { - name: "skipped for unified-host account", + name: "account failure (unified host)", cfg: &config.Config{ - Host: "https://myhost.databricks.com", + Host: unauthorized.URL, AccountID: "test-account-123", - Token: "test-token", + Token: "bad-token", Experimental_IsUnifiedHost: true, }, - wantStatus: statusSkip, - wantMsg: "account-level", + wantStatus: statusFail, + wantMsg: "Cannot list account workspaces", }, } @@ -355,11 +367,7 @@ func TestCheckIdentity(t *testing.T) { assert.Equal(t, "Identity", result.Name) assert.Equal(t, tt.wantStatus, result.Status) if tt.wantMsg != "" { - if tt.wantStatus == statusPass { - assert.Equal(t, tt.wantMsg, result.Message) - } else { - assert.Contains(t, result.Message, tt.wantMsg) - } + assert.Contains(t, result.Message, tt.wantMsg) } }) } diff --git a/cmd/doctor/extended.go b/cmd/doctor/extended.go index fec5926bb2c..0774165bc07 100644 --- a/cmd/doctor/extended.go +++ b/cmd/doctor/extended.go @@ -3,6 +3,7 @@ package doctor import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -45,7 +46,7 @@ func checkToolchain(ctx context.Context, run execFunc) CheckResult { for _, t := range tools { out, err := run(ctx, t.cmd, t.arg) if err != nil { - parts = append(parts, t.name+" not found") + parts = append(parts, t.name+" "+toolchainErrorLabel(err)) continue } parts = append(parts, t.name+" "+firstLineVersion(out)) @@ -53,6 +54,19 @@ func checkToolchain(ctx context.Context, run execFunc) CheckResult { return info("Toolchain", strings.Join(parts, ", ")) } +// toolchainErrorLabel distinguishes a missing binary from one that ran but failed, +// so users can tell "install this tool" apart from "this tool is broken". +func toolchainErrorLabel(err error) string { + if errors.Is(err, exec.ErrNotFound) { + return "not found" + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return fmt.Sprintf("exited %d", exitErr.ExitCode()) + } + return "error: " + err.Error() +} + // firstLineVersion returns a short version string from a tool's --version output. // It takes the first non-empty line and strips common program-name prefixes // (e.g. "git version 2.42.0" -> "2.42.0", "Terraform v1.5.0" -> "v1.5.0"). @@ -117,7 +131,9 @@ func checkProxy(ctx context.Context) CheckResult { return info("Proxy/TLS", strings.Join(seen, ", ")) } -// maskProxyValue hides credentials in proxy URLs (user:pass@host). +// maskProxyValue hides credentials in proxy URLs. Any userinfo (user:pass@host, +// token@host, or just user@host) is replaced with "***" since even a bare +// username can be a secret (e.g. http://TOKEN@proxy). // net/url would percent-encode "***", so the replacement is done via string rewrite // on the exact userinfo segment. func maskProxyValue(v string) string { @@ -126,11 +142,10 @@ func maskProxyValue(v string) string { return v } userinfo := u.User.String() - if _, hasPassword := u.User.Password(); !hasPassword { + if userinfo == "" { return v } - masked := u.User.Username() + ":***" - return strings.Replace(v, userinfo, masked, 1) + return strings.Replace(v, userinfo, "***", 1) } // checkUpdates fetches the latest CLI release and compares it to the current build. diff --git a/cmd/doctor/extended_test.go b/cmd/doctor/extended_test.go index 54d776cfdc3..fd7bd150994 100644 --- a/cmd/doctor/extended_test.go +++ b/cmd/doctor/extended_test.go @@ -6,6 +6,7 @@ import ( "io" "net/http" "net/http/httptest" + "os/exec" "strings" "testing" @@ -38,7 +39,8 @@ func TestCheckToolchain(t *testing.T) { return func(_ context.Context, name string, _ ...string) (string, error) { v, ok := versions[name] if !ok { - return "", errors.New("not found") + // Wrap in the same error that os/exec returns for a missing binary. + return "", &exec.Error{Name: name, Err: exec.ErrNotFound} } return v, nil } @@ -58,7 +60,7 @@ func TestCheckToolchain(t *testing.T) { assert.Contains(t, result.Message, "terraform v1.5.0") }) - t.Run("missing tools reported inline", func(t *testing.T) { + t.Run("missing tools reported as not found", func(t *testing.T) { run := mockExec(map[string]string{"git": "git version 2.42.0"}) result := checkToolchain(t.Context(), run) assert.Equal(t, statusInfo, result.Status) @@ -66,6 +68,34 @@ func TestCheckToolchain(t *testing.T) { assert.Contains(t, result.Message, "terraform not found") assert.Contains(t, result.Message, "python not found") }) + + t.Run("non-not-found errors are surfaced verbatim", func(t *testing.T) { + run := func(_ context.Context, name string, _ ...string) (string, error) { + if name == "git" { + return "", errors.New("permission denied") + } + return "", &exec.Error{Name: name, Err: exec.ErrNotFound} + } + result := checkToolchain(t.Context(), run) + assert.Contains(t, result.Message, "git error: permission denied") + assert.Contains(t, result.Message, "python not found") + }) +} + +func TestToolchainErrorLabel(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + {"not found", &exec.Error{Name: "foo", Err: exec.ErrNotFound}, "not found"}, + {"other error", errors.New("permission denied"), "error: permission denied"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, toolchainErrorLabel(tt.err)) + }) + } } func TestCheckProxy(t *testing.T) { @@ -101,8 +131,20 @@ func TestCheckProxy(t *testing.T) { ctx := cmdio.MockDiscard(t.Context()) ctx = env.Set(ctx, "HTTPS_PROXY", "http://user:secret@proxy.example.com:8080") result := checkProxy(ctx) - assert.Contains(t, result.Message, "user:***@proxy.example.com") + assert.Contains(t, result.Message, "***@proxy.example.com") assert.NotContains(t, result.Message, "secret") + assert.NotContains(t, result.Message, "user") + }) + + t.Run("masks token-only userinfo", func(t *testing.T) { + for _, v := range proxyEnvVars { + t.Setenv(v, "") + } + ctx := cmdio.MockDiscard(t.Context()) + ctx = env.Set(ctx, "HTTPS_PROXY", "http://glpat-SECRETTOKEN@proxy.example.com:8080") + result := checkProxy(ctx) + assert.Contains(t, result.Message, "***@proxy.example.com") + assert.NotContains(t, result.Message, "glpat-SECRETTOKEN") }) t.Run("deduplicates upper and lower case variants", func(t *testing.T) { @@ -123,7 +165,8 @@ func TestMaskProxyValue(t *testing.T) { in, want string }{ {"http://proxy.example.com:8080", "http://proxy.example.com:8080"}, - {"http://user:secret@proxy.example.com:8080", "http://user:***@proxy.example.com:8080"}, + {"http://user:secret@proxy.example.com:8080", "http://***@proxy.example.com:8080"}, + {"http://tokenonly@proxy.example.com:8080", "http://***@proxy.example.com:8080"}, {"not a url", "not a url"}, {"localhost,127.0.0.1", "localhost,127.0.0.1"}, } From 2401916c807df6ca652f821746f0479060663a27 Mon Sep 17 00:00:00 2001 From: simon Date: Fri, 17 Apr 2026 17:55:01 +0200 Subject: [PATCH 3/9] Move doctor under experimental Per feedback, the command is moved behind the 'experimental' subtree: databricks doctor -> databricks experimental doctor - Source: cmd/doctor/ -> experimental/doctor/cmd/ (matches the aitools convention under experimental/) - Entry point renamed New() -> NewDoctorCmd() - Hidden: true; no GroupID since it's no longer a top-level command - Registration moves from cmd/cmd.go to cmd/experimental/experimental.go - Acceptance test: acceptance/cmd/doctor -> acceptance/cmd/experimental/doctor and its script invokes 'experimental doctor' - Help output no longer lists doctor under Developer Tools - NEXT_CHANGELOG wording updated to flag the command as experimental --- NEXT_CHANGELOG.md | 2 +- acceptance/cmd/{ => experimental}/doctor/out.test.toml | 0 acceptance/cmd/{ => experimental}/doctor/output.txt | 2 +- acceptance/cmd/{ => experimental}/doctor/script | 2 +- acceptance/cmd/{ => experimental}/doctor/test.toml | 0 acceptance/help/output.txt | 1 - cmd/cmd.go | 2 -- cmd/experimental/experimental.go | 2 ++ {cmd/doctor => experimental/doctor/cmd}/checks.go | 0 {cmd/doctor => experimental/doctor/cmd}/doctor.go | 6 +++--- {cmd/doctor => experimental/doctor/cmd}/doctor_test.go | 2 +- {cmd/doctor => experimental/doctor/cmd}/extended.go | 0 {cmd/doctor => experimental/doctor/cmd}/extended_test.go | 0 13 files changed, 9 insertions(+), 10 deletions(-) rename acceptance/cmd/{ => experimental}/doctor/out.test.toml (100%) rename acceptance/cmd/{ => experimental}/doctor/output.txt (87%) rename acceptance/cmd/{ => experimental}/doctor/script (57%) rename acceptance/cmd/{ => experimental}/doctor/test.toml (100%) rename {cmd/doctor => experimental/doctor/cmd}/checks.go (100%) rename {cmd/doctor => experimental/doctor/cmd}/doctor.go (96%) rename {cmd/doctor => experimental/doctor/cmd}/doctor_test.go (99%) rename {cmd/doctor => experimental/doctor/cmd}/extended.go (100%) rename {cmd/doctor => experimental/doctor/cmd}/extended_test.go (100%) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 52af648b72d..1e5df163767 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -7,7 +7,7 @@ ### CLI * Added `--limit` flag to all paginated list commands for client-side result capping ([#4984](https://github.com/databricks/cli/pull/4984)). -* Added `databricks doctor` command that runs diagnostic checks (CLI version, updates, toolchain, proxy/TLS, log file, config file, profile, authentication, identity, network) and reports results as a checklist or JSON ([#4730](https://github.com/databricks/cli/pull/4730)). +* Added experimental `databricks experimental doctor` command that runs diagnostic checks (CLI version, updates, toolchain, proxy/TLS, log file, config file, profile, authentication, identity, network) and reports results as a checklist or JSON ([#4730](https://github.com/databricks/cli/pull/4730)). ### Bundles diff --git a/acceptance/cmd/doctor/out.test.toml b/acceptance/cmd/experimental/doctor/out.test.toml similarity index 100% rename from acceptance/cmd/doctor/out.test.toml rename to acceptance/cmd/experimental/doctor/out.test.toml diff --git a/acceptance/cmd/doctor/output.txt b/acceptance/cmd/experimental/doctor/output.txt similarity index 87% rename from acceptance/cmd/doctor/output.txt rename to acceptance/cmd/experimental/doctor/output.txt index 5c33406c137..5c755fb9c35 100644 --- a/acceptance/cmd/doctor/output.txt +++ b/acceptance/cmd/experimental/doctor/output.txt @@ -1,7 +1,7 @@ === Doctor with --profile flag ->>> [CLI] doctor --profile my-workspace --output json +>>> [CLI] experimental doctor --profile my-workspace --output json CLI Version: info - [DEV_VERSION] Updates: info - development build ([DEV_VERSION]) Toolchain: info - [TOOLCHAIN] diff --git a/acceptance/cmd/doctor/script b/acceptance/cmd/experimental/doctor/script similarity index 57% rename from acceptance/cmd/doctor/script rename to acceptance/cmd/experimental/doctor/script index 1f54c619a82..61bbd72380d 100644 --- a/acceptance/cmd/doctor/script +++ b/acceptance/cmd/experimental/doctor/script @@ -7,4 +7,4 @@ token = $DATABRICKS_TOKEN EOF title "Doctor with --profile flag\n" -trace $CLI doctor --profile my-workspace --output json | jq -r '.[] | "\(.name): \(.status) - \(.message)"' +trace $CLI experimental doctor --profile my-workspace --output json | jq -r '.[] | "\(.name): \(.status) - \(.message)"' diff --git a/acceptance/cmd/doctor/test.toml b/acceptance/cmd/experimental/doctor/test.toml similarity index 100% rename from acceptance/cmd/doctor/test.toml rename to acceptance/cmd/experimental/doctor/test.toml diff --git a/acceptance/help/output.txt b/acceptance/help/output.txt index b1c6d7adaa2..d9f379f5bbf 100644 --- a/acceptance/help/output.txt +++ b/acceptance/help/output.txt @@ -163,7 +163,6 @@ Environments Developer Tools bundle Declarative Automation Bundles let you express data/AI/analytics projects as code. - doctor Validate your Databricks CLI setup sync Synchronize a local directory to a workspace directory Additional Commands: diff --git a/cmd/cmd.go b/cmd/cmd.go index 217476f4505..014471f7638 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -14,7 +14,6 @@ import ( "github.com/databricks/cli/cmd/cache" "github.com/databricks/cli/cmd/completion" "github.com/databricks/cli/cmd/configure" - "github.com/databricks/cli/cmd/doctor" "github.com/databricks/cli/cmd/experimental" "github.com/databricks/cli/cmd/fs" "github.com/databricks/cli/cmd/labs" @@ -102,7 +101,6 @@ func New(ctx context.Context) *cobra.Command { cli.AddCommand(experimental.New()) cli.AddCommand(psql.New()) cli.AddCommand(configure.New()) - cli.AddCommand(doctor.New()) cli.AddCommand(fs.New()) cli.AddCommand(labs.New(ctx)) cli.AddCommand(sync.New()) diff --git a/cmd/experimental/experimental.go b/cmd/experimental/experimental.go index eb3b7814e1a..26683c4caa5 100644 --- a/cmd/experimental/experimental.go +++ b/cmd/experimental/experimental.go @@ -2,6 +2,7 @@ package experimental import ( aitoolscmd "github.com/databricks/cli/experimental/aitools/cmd" + doctorcmd "github.com/databricks/cli/experimental/doctor/cmd" "github.com/spf13/cobra" ) @@ -21,6 +22,7 @@ development. They may change or be removed in future versions without notice.`, } cmd.AddCommand(aitoolscmd.NewAitoolsCmd()) + cmd.AddCommand(doctorcmd.NewDoctorCmd()) return cmd } diff --git a/cmd/doctor/checks.go b/experimental/doctor/cmd/checks.go similarity index 100% rename from cmd/doctor/checks.go rename to experimental/doctor/cmd/checks.go diff --git a/cmd/doctor/doctor.go b/experimental/doctor/cmd/doctor.go similarity index 96% rename from cmd/doctor/doctor.go rename to experimental/doctor/cmd/doctor.go index 3d55341da6e..f59a0c742b4 100644 --- a/cmd/doctor/doctor.go +++ b/experimental/doctor/cmd/doctor.go @@ -20,13 +20,13 @@ type CheckResult struct { Detail string `json:"detail,omitempty"` } -// New returns the doctor command. -func New() *cobra.Command { +// NewDoctorCmd returns the doctor command. +func NewDoctorCmd() *cobra.Command { cmd := &cobra.Command{ Use: "doctor", Args: root.NoArgs, Short: "Validate your Databricks CLI setup", - GroupID: "development", + Hidden: true, SilenceUsage: true, SilenceErrors: true, } diff --git a/cmd/doctor/doctor_test.go b/experimental/doctor/cmd/doctor_test.go similarity index 99% rename from cmd/doctor/doctor_test.go rename to experimental/doctor/cmd/doctor_test.go index 06833817911..bf5fc8075d0 100644 --- a/cmd/doctor/doctor_test.go +++ b/experimental/doctor/cmd/doctor_test.go @@ -582,7 +582,7 @@ func TestNewCommandJSON(t *testing.T) { }, }) - cmd := New() + cmd := NewDoctorCmd() cmd.SetContext(ctx) outputFlag := flags.OutputText diff --git a/cmd/doctor/extended.go b/experimental/doctor/cmd/extended.go similarity index 100% rename from cmd/doctor/extended.go rename to experimental/doctor/cmd/extended.go diff --git a/cmd/doctor/extended_test.go b/experimental/doctor/cmd/extended_test.go similarity index 100% rename from cmd/doctor/extended_test.go rename to experimental/doctor/cmd/extended_test.go From e0b9d5f6c133fd7bc46d9b69b2195172119ae95d Mon Sep 17 00:00:00 2001 From: simon Date: Fri, 17 Apr 2026 17:57:00 +0200 Subject: [PATCH 4/9] Drop NEXT_CHANGELOG entry for experimental doctor Experimental commands don't go into the release notes until they graduate. No CI check requires an entry, so this file matches main now. --- NEXT_CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 1e5df163767..52d764e384d 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -7,7 +7,6 @@ ### CLI * Added `--limit` flag to all paginated list commands for client-side result capping ([#4984](https://github.com/databricks/cli/pull/4984)). -* Added experimental `databricks experimental doctor` command that runs diagnostic checks (CLI version, updates, toolchain, proxy/TLS, log file, config file, profile, authentication, identity, network) and reports results as a checklist or JSON ([#4730](https://github.com/databricks/cli/pull/4730)). ### Bundles From ebf48c339f0765533d93363a7c2c1b353b455f30 Mon Sep 17 00:00:00 2001 From: simon Date: Tue, 21 Apr 2026 10:27:02 +0200 Subject: [PATCH 5/9] Address Renaud's review on doctor command - Wrap JSON output in DoctorReport{Results: ...} so we can add fields later without breaking callers of the array shape. - Type the status enum (type status string) so misuse is a compile error, while keeping the string JSON wire contract. - Make CheckResult.Detail any so future structured details don't require a breaking change. - Add omitempty to Name/Status/Message so empty results render as {}. - Inline runChecks into a composite literal to avoid append resizes. - Replace if/return chain in checkCurrentProfile with a switch. Co-authored-by: Isaac --- acceptance/cmd/experimental/doctor/script | 2 +- experimental/doctor/cmd/checks.go | 53 +++++++++-------------- experimental/doctor/cmd/doctor.go | 33 +++++++++++--- experimental/doctor/cmd/doctor_test.go | 24 +++++----- 4 files changed, 60 insertions(+), 52 deletions(-) diff --git a/acceptance/cmd/experimental/doctor/script b/acceptance/cmd/experimental/doctor/script index 61bbd72380d..9a74e060b2f 100644 --- a/acceptance/cmd/experimental/doctor/script +++ b/acceptance/cmd/experimental/doctor/script @@ -7,4 +7,4 @@ token = $DATABRICKS_TOKEN EOF title "Doctor with --profile flag\n" -trace $CLI experimental doctor --profile my-workspace --output json | jq -r '.[] | "\(.name): \(.status) - \(.message)"' +trace $CLI experimental doctor --profile my-workspace --output json | jq -r '.results[] | "\(.name): \(.status) - \(.message)"' diff --git a/experimental/doctor/cmd/checks.go b/experimental/doctor/cmd/checks.go index f17d344c82a..166002a0f98 100644 --- a/experimental/doctor/cmd/checks.go +++ b/experimental/doctor/cmd/checks.go @@ -20,12 +20,6 @@ import ( ) const ( - statusPass = "pass" - statusFail = "fail" - statusWarn = "warn" - statusInfo = "info" - statusSkip = "skip" - networkTimeout = 10 * time.Second checkTimeout = 15 * time.Second ) @@ -61,27 +55,25 @@ func withCheckTimeout(ctx context.Context) (context.Context, context.CancelFunc) // runChecks runs all diagnostic checks and returns the results. func runChecks(ctx context.Context, profile string, profileFromFlag bool) []CheckResult { cfg, err := resolveConfig(ctx, profile, profileFromFlag) - - var results []CheckResult - results = append(results, checkCLIVersion()) - results = append(results, checkUpdates(ctx, http.DefaultClient, updateCheckURL)) - results = append(results, checkToolchain(ctx, realExec)) - results = append(results, checkProxy(ctx)) - results = append(results, checkLogFile(ctx)) - results = append(results, checkConfigFile(ctx)) - results = append(results, checkCurrentProfile(ctx, profile, profileFromFlag, cfg)) - authResult, authCfg := checkAuth(ctx, cfg, err) - results = append(results, authResult) + identityResult := skip("Identity", "Skipped (authentication failed)") if authCfg != nil { - results = append(results, checkIdentity(ctx, authCfg)) - } else { - results = append(results, skip("Identity", "Skipped (authentication failed)")) + identityResult = checkIdentity(ctx, authCfg) } - results = append(results, checkNetwork(ctx, cfg, err, authCfg)) - return results + return []CheckResult{ + checkCLIVersion(), + checkUpdates(ctx, http.DefaultClient, updateCheckURL), + checkToolchain(ctx, realExec), + checkProxy(ctx), + checkLogFile(ctx), + checkConfigFile(ctx), + checkCurrentProfile(ctx, profile, profileFromFlag, cfg), + authResult, + identityResult, + checkNetwork(ctx, cfg, err, authCfg), + } } func checkCLIVersion() CheckResult { @@ -109,20 +101,17 @@ func checkConfigFile(ctx context.Context) CheckResult { } func checkCurrentProfile(ctx context.Context, profileName string, fromFlag bool, resolvedCfg *config.Config) CheckResult { - if fromFlag { + switch envProfile := env.Get(ctx, "DATABRICKS_CONFIG_PROFILE"); { + case fromFlag: return info("Current Profile", profileName) - } - - if envProfile := env.Get(ctx, "DATABRICKS_CONFIG_PROFILE"); envProfile != "" { + case envProfile != "": return info("Current Profile", envProfile+" (from DATABRICKS_CONFIG_PROFILE)") - } - - // Check if the SDK resolved a profile (e.g. DEFAULT section in .databrickscfg). - if resolvedCfg != nil && resolvedCfg.Profile != "" { + // The SDK resolves a profile when DEFAULT is in .databrickscfg. + case resolvedCfg != nil && resolvedCfg.Profile != "": return info("Current Profile", resolvedCfg.Profile+" (resolved from config file)") + default: + return info("Current Profile", "none (using environment or defaults)") } - - return info("Current Profile", "none (using environment or defaults)") } func resolveConfig(ctx context.Context, profileName string, fromFlag bool) (*config.Config, error) { diff --git a/experimental/doctor/cmd/doctor.go b/experimental/doctor/cmd/doctor.go index f59a0c742b4..a69ebaed75d 100644 --- a/experimental/doctor/cmd/doctor.go +++ b/experimental/doctor/cmd/doctor.go @@ -12,12 +12,31 @@ import ( "github.com/spf13/cobra" ) +// status identifies a CheckResult's outcome. The string values are part of the +// JSON wire contract emitted by --output json. +type status string + +const ( + statusPass status = "pass" + statusFail status = "fail" + statusWarn status = "warn" + statusInfo status = "info" + statusSkip status = "skip" +) + // CheckResult holds the outcome of a single diagnostic check. type CheckResult struct { - Name string `json:"name"` - Status string `json:"status"` // "pass", "fail", "warn", "info", "skip" - Message string `json:"message"` - Detail string `json:"detail,omitempty"` + Name string `json:"name,omitempty"` + Status status `json:"status,omitempty"` + Message string `json:"message,omitempty"` + Detail any `json:"detail,omitempty"` +} + +// DoctorReport is the top-level JSON output shape. Wrapping the results in an +// object leaves room to add fields (summary, version, durationMs, ...) without +// breaking callers that already parse the response. +type DoctorReport struct { + Results []CheckResult `json:"results"` } // NewDoctorCmd returns the doctor command. @@ -59,7 +78,7 @@ func profileFromCommand(cmd *cobra.Command) (string, bool) { func render(w io.Writer, results []CheckResult, outputType flags.Output) error { switch outputType { case flags.OutputJSON: - buf, err := json.MarshalIndent(results, "", " ") + buf, err := json.MarshalIndent(DoctorReport{Results: results}, "", " ") if err != nil { return err } @@ -96,8 +115,8 @@ func renderText(w io.Writer, results []CheckResult) { icon = yellow("[skip]") } msg := fmt.Sprintf("%s %s: %s", icon, bold(r.Name), r.Message) - if r.Detail != "" { - msg += fmt.Sprintf(" (%s)", r.Detail) + if r.Detail != nil { + msg += fmt.Sprintf(" (%v)", r.Detail) } fmt.Fprintln(w, msg) } diff --git a/experimental/doctor/cmd/doctor_test.go b/experimental/doctor/cmd/doctor_test.go index bf5fc8075d0..7e3aeb4071e 100644 --- a/experimental/doctor/cmd/doctor_test.go +++ b/experimental/doctor/cmd/doctor_test.go @@ -87,7 +87,7 @@ func TestCheckConfigFile(t *testing.T) { tests := []struct { name string profiler profile.Profiler - wantStatus string + wantStatus status wantMsg string }{ { @@ -190,7 +190,7 @@ func TestCheckAuth(t *testing.T) { tests := []struct { name string env map[string]string - wantStatus string + wantStatus status wantMsgPart string wantAuthCfg bool }{ @@ -322,7 +322,7 @@ func TestCheckIdentity(t *testing.T) { tests := []struct { name string cfg *config.Config - wantStatus string + wantStatus status wantMsg string }{ { @@ -530,11 +530,11 @@ func TestRender(t *testing.T) { require.NoError(t, render(&buf, results, flags.OutputJSON)) assert.Equal(t, byte('\n'), buf.Bytes()[buf.Len()-1]) - var parsed []CheckResult + var parsed DoctorReport require.NoError(t, json.Unmarshal(buf.Bytes(), &parsed)) - assert.Len(t, parsed, 4) - assert.Equal(t, "Test", parsed[0].Name) - assert.Equal(t, "details here", parsed[1].Detail) + assert.Len(t, parsed.Results, 4) + assert.Equal(t, "Test", parsed.Results[0].Name) + assert.Equal(t, "details here", parsed.Results[1].Detail) }) t.Run("json omits empty detail", func(t *testing.T) { @@ -596,9 +596,9 @@ func TestNewCommandJSON(t *testing.T) { require.ErrorContains(t, err, "one or more checks failed") assert.Equal(t, byte('\n'), buf.Bytes()[buf.Len()-1]) - var results []CheckResult - require.NoError(t, json.Unmarshal(buf.Bytes(), &results)) - assert.GreaterOrEqual(t, len(results), 4) - assert.Equal(t, "CLI Version", results[0].Name) - assert.Equal(t, statusInfo, results[0].Status) + var report DoctorReport + require.NoError(t, json.Unmarshal(buf.Bytes(), &report)) + assert.GreaterOrEqual(t, len(report.Results), 4) + assert.Equal(t, "CLI Version", report.Results[0].Name) + assert.Equal(t, statusInfo, report.Results[0].Status) } From d140a2b446cf6b8cbbfe66ed0d9372d046584c2c Mon Sep 17 00:00:00 2001 From: simon Date: Tue, 21 Apr 2026 12:43:12 +0200 Subject: [PATCH 6/9] Silence fatcontext lint in doctor_test TestResolveConfig Co-authored-by: Isaac --- experimental/doctor/cmd/doctor_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experimental/doctor/cmd/doctor_test.go b/experimental/doctor/cmd/doctor_test.go index 7e3aeb4071e..d3873ec1550 100644 --- a/experimental/doctor/cmd/doctor_test.go +++ b/experimental/doctor/cmd/doctor_test.go @@ -282,7 +282,7 @@ func TestResolveConfig(t *testing.T) { } ctx := cmdio.MockDiscard(t.Context()) for k, v := range tt.ctxEnv { - ctx = env.Set(ctx, k, v) + ctx = env.Set(ctx, k, v) //nolint:fatcontext // accumulating test env vars onto context } cfg, err := resolveConfig(ctx, "", false) From edb3ccfc8fb7f3ef1bdea4d765e109b20838773c Mon Sep 17 00:00:00 2001 From: simon Date: Wed, 22 Apr 2026 16:09:25 +0200 Subject: [PATCH 7/9] Address Renaud's second-pass review on doctor command - Reword isAccountLevelConfig godoc to "can target" so callers don't read it as "account-exclusive". - Drop the withCheckTimeout wrapper and inline context.WithTimeout at call sites; the wrapper was not adding readability. - Move config-resolution error handling out of checkAuth and into runChecks, so checkAuth is only responsible for authenticating a resolved config. Co-authored-by: Isaac --- experimental/doctor/cmd/checks.go | 31 +++++++++++++------------- experimental/doctor/cmd/doctor_test.go | 5 +++-- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/experimental/doctor/cmd/checks.go b/experimental/doctor/cmd/checks.go index 166002a0f98..8922113e19f 100644 --- a/experimental/doctor/cmd/checks.go +++ b/experimental/doctor/cmd/checks.go @@ -48,14 +48,19 @@ func fail(name, msg string, err error) CheckResult { return r } -func withCheckTimeout(ctx context.Context) (context.Context, context.CancelFunc) { - return context.WithTimeout(ctx, checkTimeout) -} - // runChecks runs all diagnostic checks and returns the results. func runChecks(ctx context.Context, profile string, profileFromFlag bool) []CheckResult { - cfg, err := resolveConfig(ctx, profile, profileFromFlag) - authResult, authCfg := checkAuth(ctx, cfg, err) + cfg, resolveErr := resolveConfig(ctx, profile, profileFromFlag) + + var ( + authResult CheckResult + authCfg *config.Config + ) + if resolveErr != nil { + authResult = fail("Authentication", "Cannot resolve config", resolveErr) + } else { + authResult, authCfg = checkAuth(ctx, cfg) + } identityResult := skip("Identity", "Skipped (authentication failed)") if authCfg != nil { @@ -72,7 +77,7 @@ func runChecks(ctx context.Context, profile string, profileFromFlag bool) []Chec checkCurrentProfile(ctx, profile, profileFromFlag, cfg), authResult, identityResult, - checkNetwork(ctx, cfg, err, authCfg), + checkNetwork(ctx, cfg, resolveErr, authCfg), } } @@ -128,7 +133,7 @@ func resolveConfig(ctx context.Context, profileName string, fromFlag bool) (*con return cfg, cfg.EnsureResolved() } -// isAccountLevelConfig returns true if the resolved config targets account-level APIs. +// isAccountLevelConfig returns true if the resolved config can target account-level APIs. // It uses auth.ResolveConfigType so SPOG / unified-host profiles, which the SDK's own // ConfigType() classifies as InvalidConfig, are still recognised as account-level. func isAccountLevelConfig(cfg *config.Config) bool { @@ -137,14 +142,10 @@ func isAccountLevelConfig(cfg *config.Config) bool { // checkAuth uses the resolved config to authenticate. // On success it returns the authenticated config for use in subsequent checks. -func checkAuth(ctx context.Context, cfg *config.Config, resolveErr error) (CheckResult, *config.Config) { - ctx, cancel := withCheckTimeout(ctx) +func checkAuth(ctx context.Context, cfg *config.Config) (CheckResult, *config.Config) { + ctx, cancel := context.WithTimeout(ctx, checkTimeout) defer cancel() - if resolveErr != nil { - return fail("Authentication", "Cannot resolve config", resolveErr), nil - } - // Detect account-level configs and use the appropriate client constructor // so that account profiles are not incorrectly reported as broken. var authCfg *config.Config @@ -179,7 +180,7 @@ func checkAuth(ctx context.Context, cfg *config.Config, resolveErr error) (Check } func checkIdentity(ctx context.Context, authCfg *config.Config) CheckResult { - ctx, cancel := withCheckTimeout(ctx) + ctx, cancel := context.WithTimeout(ctx, checkTimeout) defer cancel() if isAccountLevelConfig(authCfg) { diff --git a/experimental/doctor/cmd/doctor_test.go b/experimental/doctor/cmd/doctor_test.go index d3873ec1550..978eef10686 100644 --- a/experimental/doctor/cmd/doctor_test.go +++ b/experimental/doctor/cmd/doctor_test.go @@ -229,8 +229,9 @@ func TestCheckAuth(t *testing.T) { } ctx := cmdio.MockDiscard(t.Context()) - cfg, resolveErr := resolveConfig(ctx, "", false) - result, authCfg := checkAuth(ctx, cfg, resolveErr) + cfg, err := resolveConfig(ctx, "", false) + require.NoError(t, err) + result, authCfg := checkAuth(ctx, cfg) assert.Equal(t, "Authentication", result.Name) assert.Equal(t, tt.wantStatus, result.Status) From 241109abc3b2324589a6c5e4e869cc76e6e76699 Mon Sep 17 00:00:00 2001 From: simon Date: Fri, 24 Apr 2026 16:03:09 +0200 Subject: [PATCH 8/9] doctor: pass profiler directly instead of via context WithProfiler was removed as dead code on main (PR #4974). Match the rest of the doctor command by injecting the profiler directly into checkConfigFile, the same way exec and http.Client are injected into the other checks. --- experimental/doctor/cmd/checks.go | 12 +++++------- experimental/doctor/cmd/doctor_test.go | 10 ++-------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/experimental/doctor/cmd/checks.go b/experimental/doctor/cmd/checks.go index 8922113e19f..d312ec76f8c 100644 --- a/experimental/doctor/cmd/checks.go +++ b/experimental/doctor/cmd/checks.go @@ -49,8 +49,8 @@ func fail(name, msg string, err error) CheckResult { } // runChecks runs all diagnostic checks and returns the results. -func runChecks(ctx context.Context, profile string, profileFromFlag bool) []CheckResult { - cfg, resolveErr := resolveConfig(ctx, profile, profileFromFlag) +func runChecks(ctx context.Context, profileName string, profileFromFlag bool) []CheckResult { + cfg, resolveErr := resolveConfig(ctx, profileName, profileFromFlag) var ( authResult CheckResult @@ -73,8 +73,8 @@ func runChecks(ctx context.Context, profile string, profileFromFlag bool) []Chec checkToolchain(ctx, realExec), checkProxy(ctx), checkLogFile(ctx), - checkConfigFile(ctx), - checkCurrentProfile(ctx, profile, profileFromFlag, cfg), + checkConfigFile(ctx, profile.DefaultProfiler), + checkCurrentProfile(ctx, profileName, profileFromFlag, cfg), authResult, identityResult, checkNetwork(ctx, cfg, resolveErr, authCfg), @@ -85,9 +85,7 @@ func checkCLIVersion() CheckResult { return info("CLI Version", build.GetInfo().Version) } -func checkConfigFile(ctx context.Context) CheckResult { - profiler := profile.GetProfiler(ctx) - +func checkConfigFile(ctx context.Context, profiler profile.Profiler) CheckResult { path, err := profiler.GetPath(ctx) if err != nil { return fail("Config File", "Cannot determine config file path", err) diff --git a/experimental/doctor/cmd/doctor_test.go b/experimental/doctor/cmd/doctor_test.go index 978eef10686..7afdc4f4e06 100644 --- a/experimental/doctor/cmd/doctor_test.go +++ b/experimental/doctor/cmd/doctor_test.go @@ -117,8 +117,8 @@ func TestCheckConfigFile(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := profile.WithProfiler(cmdio.MockDiscard(t.Context()), tt.profiler) - result := checkConfigFile(ctx) + ctx := cmdio.MockDiscard(t.Context()) + result := checkConfigFile(ctx, tt.profiler) assert.Equal(t, "Config File", result.Name) assert.Equal(t, tt.wantStatus, result.Status) if tt.wantMsg != "" { @@ -576,12 +576,6 @@ func TestNewCommandJSON(t *testing.T) { clearConfigEnv(t) ctx := cmdio.MockDiscard(t.Context()) - ctx = profile.WithProfiler(ctx, &mockProfiler{ - path: "/tmp/.databrickscfg", - profiles: profile.Profiles{ - {Name: "default", Host: "https://example.com"}, - }, - }) cmd := NewDoctorCmd() cmd.SetContext(ctx) From 955ef1689c2ae23cc14ff96765e6d0e57b8bc4ce Mon Sep 17 00:00:00 2001 From: simon Date: Fri, 15 May 2026 08:49:56 +0200 Subject: [PATCH 9/9] update doctor for post-merge breakage from main Two changes landed on main that broke the doctor branch: 1. Experimental_IsUnifiedHost was removed from the SDK Config (#5047). Update the doctor tests to express unified-host detection via DiscoveryURL containing /oidc/accounts/, matching the new auth.IsSPOG / HasUnifiedHostSignal mechanism. 2. fatih/color was replaced with in-tree cmdio ANSI helpers (#5178). Migrate doctor renderText to use cmdio.Red/Green/Yellow/Cyan/Bold. Adds a Bold helper to libs/cmdio/color.go since the existing migration didn't need bold but the doctor output does. Co-authored-by: Isaac --- experimental/doctor/cmd/doctor.go | 29 ++++++++----------- experimental/doctor/cmd/doctor_test.go | 40 +++++++++++++------------- libs/cmdio/color.go | 3 ++ 3 files changed, 35 insertions(+), 37 deletions(-) diff --git a/experimental/doctor/cmd/doctor.go b/experimental/doctor/cmd/doctor.go index a69ebaed75d..7f99990f492 100644 --- a/experimental/doctor/cmd/doctor.go +++ b/experimental/doctor/cmd/doctor.go @@ -1,14 +1,15 @@ package doctor import ( + "context" "encoding/json" "errors" "fmt" "io" "github.com/databricks/cli/cmd/root" + "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/flags" - "github.com/fatih/color" "github.com/spf13/cobra" ) @@ -54,7 +55,7 @@ func NewDoctorCmd() *cobra.Command { profileName, fromFlag := profileFromCommand(cmd) results := runChecks(cmd.Context(), profileName, fromFlag) - if err := render(cmd.OutOrStdout(), results, root.OutputType(cmd)); err != nil { + if err := render(cmd.Context(), cmd.OutOrStdout(), results, root.OutputType(cmd)); err != nil { return err } @@ -75,7 +76,7 @@ func profileFromCommand(cmd *cobra.Command) (string, bool) { return f.Value.String(), true } -func render(w io.Writer, results []CheckResult, outputType flags.Output) error { +func render(ctx context.Context, w io.Writer, results []CheckResult, outputType flags.Output) error { switch outputType { case flags.OutputJSON: buf, err := json.MarshalIndent(DoctorReport{Results: results}, "", " ") @@ -86,35 +87,29 @@ func render(w io.Writer, results []CheckResult, outputType flags.Output) error { _, err = w.Write(buf) return err case flags.OutputText: - renderText(w, results) + renderText(ctx, w, results) return nil default: return fmt.Errorf("unknown output type %s", outputType) } } -func renderText(w io.Writer, results []CheckResult) { - green := color.New(color.FgGreen).SprintFunc() - red := color.New(color.FgRed).SprintFunc() - yellow := color.New(color.FgYellow).SprintFunc() - cyan := color.New(color.FgCyan).SprintFunc() - bold := color.New(color.Bold).SprintFunc() - +func renderText(ctx context.Context, w io.Writer, results []CheckResult) { for _, r := range results { var icon string switch r.Status { case statusPass: - icon = green("[ok]") + icon = cmdio.Green(ctx, "[ok]") case statusFail: - icon = red("[FAIL]") + icon = cmdio.Red(ctx, "[FAIL]") case statusWarn: - icon = yellow("[warn]") + icon = cmdio.Yellow(ctx, "[warn]") case statusInfo: - icon = cyan("[info]") + icon = cmdio.Cyan(ctx, "[info]") case statusSkip: - icon = yellow("[skip]") + icon = cmdio.Yellow(ctx, "[skip]") } - msg := fmt.Sprintf("%s %s: %s", icon, bold(r.Name), r.Message) + msg := fmt.Sprintf("%s %s: %s", icon, cmdio.Bold(ctx, r.Name), r.Message) if r.Detail != nil { msg += fmt.Sprintf(" (%v)", r.Detail) } diff --git a/experimental/doctor/cmd/doctor_test.go b/experimental/doctor/cmd/doctor_test.go index 7afdc4f4e06..3c3089d4bc4 100644 --- a/experimental/doctor/cmd/doctor_test.go +++ b/experimental/doctor/cmd/doctor_test.go @@ -340,10 +340,10 @@ func TestCheckIdentity(t *testing.T) { { name: "account success (unified host)", cfg: &config.Config{ - Host: accountOK.URL, - AccountID: "test-account-123", - Token: "test-token", - Experimental_IsUnifiedHost: true, + Host: accountOK.URL, + AccountID: "test-account-123", + Token: "test-token", + DiscoveryURL: "https://spog.databricks.test/oidc/accounts/test-account-123/.well-known/oauth-authorization-server", }, wantStatus: statusPass, wantMsg: "account test-account-123 (2 workspaces)", @@ -351,10 +351,10 @@ func TestCheckIdentity(t *testing.T) { { name: "account failure (unified host)", cfg: &config.Config{ - Host: unauthorized.URL, - AccountID: "test-account-123", - Token: "bad-token", - Experimental_IsUnifiedHost: true, + Host: unauthorized.URL, + AccountID: "test-account-123", + Token: "bad-token", + DiscoveryURL: "https://spog.databricks.test/oidc/accounts/test-account-123/.well-known/oauth-authorization-server", }, wantStatus: statusFail, wantMsg: "Cannot list account workspaces", @@ -391,27 +391,27 @@ func TestIsAccountLevelConfig(t *testing.T) { { name: "unified host with account ID", cfg: &config.Config{ - Host: "https://myhost.databricks.com", - AccountID: "test-account-123", - Experimental_IsUnifiedHost: true, + Host: "https://myhost.databricks.test", + AccountID: "test-account-123", + DiscoveryURL: "https://myhost.databricks.test/oidc/accounts/test-account-123/.well-known/oauth-authorization-server", }, want: true, }, { name: "unified host with account and workspace ID is workspace-level", cfg: &config.Config{ - Host: "https://myhost.databricks.com", - AccountID: "test-account-123", - WorkspaceID: "12345", - Experimental_IsUnifiedHost: true, + Host: "https://myhost.databricks.test", + AccountID: "test-account-123", + WorkspaceID: "12345", + DiscoveryURL: "https://myhost.databricks.test/oidc/accounts/test-account-123/.well-known/oauth-authorization-server", }, want: false, }, { name: "unified host without account ID is workspace", cfg: &config.Config{ - Host: "https://myhost.databricks.com", - Experimental_IsUnifiedHost: true, + Host: "https://myhost.databricks.test", + DiscoveryURL: "https://myhost.databricks.test/oidc/accounts/test-account-123/.well-known/oauth-authorization-server", }, want: false, }, @@ -518,7 +518,7 @@ func TestRender(t *testing.T) { t.Run("text", func(t *testing.T) { var buf bytes.Buffer - require.NoError(t, render(&buf, results, flags.OutputText)) + require.NoError(t, render(t.Context(), &buf, results, flags.OutputText)) out := buf.String() assert.Contains(t, out, "Test") assert.Contains(t, out, "all good") @@ -528,7 +528,7 @@ func TestRender(t *testing.T) { t.Run("json round-trips and ends with newline", func(t *testing.T) { var buf bytes.Buffer - require.NoError(t, render(&buf, results, flags.OutputJSON)) + require.NoError(t, render(t.Context(), &buf, results, flags.OutputJSON)) assert.Equal(t, byte('\n'), buf.Bytes()[buf.Len()-1]) var parsed DoctorReport @@ -540,7 +540,7 @@ func TestRender(t *testing.T) { t.Run("json omits empty detail", func(t *testing.T) { var buf bytes.Buffer - require.NoError(t, render(&buf, []CheckResult{{Name: "Test", Status: statusPass, Message: "ok"}}, flags.OutputJSON)) + require.NoError(t, render(t.Context(), &buf, []CheckResult{{Name: "Test", Status: statusPass, Message: "ok"}}, flags.OutputJSON)) assert.NotContains(t, buf.String(), "detail") }) } diff --git a/libs/cmdio/color.go b/libs/cmdio/color.go index ffece82e47e..850a3bc4b68 100644 --- a/libs/cmdio/color.go +++ b/libs/cmdio/color.go @@ -44,6 +44,9 @@ func render(ctx context.Context, code, msg string) string { return code + msg + ansiReset } +// Bold renders msg in bold. +func Bold(ctx context.Context, msg string) string { return render(ctx, ansiBold, msg) } + // Red renders msg in red. func Red(ctx context.Context, msg string) string { return render(ctx, ansiRed, msg) }