[CFX-7585] Stop reporting server errors and bad endpoint schemes as credential problems

[chasdr]

Summary

dr told users to throw away a working API token whenever the version check came back as anything other than 200. A 404 from a mistyped endpoint, a 429, or a 503 all printed "DATAROBOT_API_TOKEN is invalid or expired, unset it". Only 401 and 403 still say that; any other status now names what the instance answered. And an endpoint scheme the CLI cannot use (ftp://host) reads as a bad endpoint instead of a network failure. Both were suppressed review findings on #751, which made them visible on every command.

Notes for review

The scheme check went into ValidateEndpoint, not into the SchemeHostOnly helper that set-url and export share, so those keep accepting what they always did.

Deliberately out of scope, same defect family, follow-up material: dr auth check's .env and stored-profile legs still call every failure a stale token or missing key, EnsureAuthenticated still starts the login flow when the stored profile's instance answers 5xx, and dr auth set-url ftp://host still writes the endpoint unvalidated.

Output

$ DATAROBOT_ENDPOINT=http://127.0.0.1:51801/api/v2 dr auth check   # instance returning 503
- ❌ DATAROBOT_API_TOKEN environment variable is invalid or expired.
- Unset it and try again:
-   unset DATAROBOT_API_TOKEN (or Remove-Item Env:\DATAROBOT_API_TOKEN on Windows)
+ ❌ http://127.0.0.1:51801 answered HTTP 503, so the CLI could not verify your credentials.
+ Check DATAROBOT_ENDPOINT, and the instance's status if it persists.

$ DATAROBOT_ENDPOINT=ftp://example.com/api/v2 dr auth check
- ❌ Could not connect to ftp://example.com: unsupported protocol scheme "ftp"
- Check DATAROBOT_ENDPOINT and your network, then try again.
+ ❌ DATAROBOT_ENDPOINT environment variable is invalid: unsupported URL scheme "ftp", use https://
+ Set it to a valid DataRobot URL and try again.

Technical Changes

• internal/config/auth.go: VerifyToken returns *HTTPStatusError{StatusCode} instead of a flat invalid token.
• internal/auth/auth.go: ValidateEndpoint requires http or https; the classifier's fallthrough splits on the status.
• Tests pin the status preservation, its consumers (GetAPIKey, the .env leg of dr auth check), and 401/403 keeping the exact [CFX-7263] Fail on bad env credentials instead of using the stored profile #751 wording.
• docs/: the two new messages.

---

Note

Medium Risk
Changes shared auth verification and error reporting used on every authenticated command; behavior is narrower (better classification) but mistakes could mis-route users on edge HTTP statuses.

Overview
Fixes misleading “invalid or expired API token” messages when environment credentials fail for reasons other than rejected auth.

VerifyToken now returns *HTTPStatusError with the real status instead of a generic invalid-token error. ReportEnvCredentialsError (used by EnsureAuthenticated and dr auth check for env vars) only tells users to unset DATAROBOT_API_TOKEN on 401 or 403; for other non-2xx responses it reports that the instance answered HTTP n and points at endpoint/instance health. ValidateEndpoint additionally rejects schemes other than http/https (e.g. ftp://) as a bad endpoint, not a connection failure.

Docs and tests cover status preservation, GetAPIKey, scheme validation, and the new user-facing copy. The .env path in dr auth check and stored-profile login behavior are unchanged for non-401 failures (called out in code as follow-up).

^{Reviewed by Cursor Bugbot for commit 54b5b4a. Configure here.}

[datarobot-pr-review-router]

🎫 Jira: CFX-7585 — dr misdiagnoses server errors and non-http schemes as credential problems

[chasdr]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 489b510. Configure here.}

[chasdr]

bugbot run

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (3)

cmd/auth/check/cmd.go:148

• This pre-validation bypasses the shared reporter, so a .env endpoint such as ftp://... only emits the generic “Invalid DATAROBOT_ENDPOINT” message and drops the unsupported-scheme reason. That contradicts the PR/docs claim that all three credential sources use the same classification and source-specific scheme message. Let verification fail and route the error through ReportUnjudged, as the block below already does.

	if err := auth.ValidateEndpoint(dotenvEndpoint); err != nil {
		fmt.Fprintln(w, tui.BaseTextStyle.Render("❌ Invalid DATAROBOT_ENDPOINT in '.env'."))
		fmt.Fprint(w, tui.BaseTextStyle.Render("Run "))
		fmt.Fprint(w, tui.InfoStyle.Render("dr dotenv update"))
		fmt.Fprintln(w, tui.BaseTextStyle.Render(" to fix the configuration."))

cmd/auth/check/cmd.go:71

• When no stored endpoint is configured, GetAPIKey returns a *url.Error for the relative /version/ request, so this now prints Could not connect to : unsupported protocol scheme "" immediately after the correct “No DataRobot URL configured” message. Guard this classification with the already-computed datarobotHost so the missing-URL case does not masquerade as a transport failure.

		if !auth.ReportUnjudged(w, viperx.GetString(config.DataRobotURL), auth.StoredEndpointName, err) {

internal/auth/auth.go:323

• This duplicates the newly exported config.HTTPStatusText implementation verbatim. Using that helper here and removing the local copy keeps unknown-status formatting defined in one place and prevents the two user-facing paths from drifting.

// httpStatusText renders "HTTP 503 Service Unavailable", dropping the name for
// codes Go does not know (Cloudflare 520-527) so no dangling space is left.
func httpStatusText(code int) string {
	if name := http.StatusText(code); name != "" {
		return fmt.Sprintf("HTTP %d %s", code, name)

[chasdr]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit ef08abb. Configure here.}

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

docs/development/authentication.md:32

• This says every 2xx response verifies successfully, but VerifyToken accepts only HTTP 200 and returns HTTPStatusError for all other statuses. Describe this as a non-200 status so the developer documentation matches the actual contract.

1. **Checks environment credentials first**: A complete `DATAROBOT_ENDPOINT` (or `DATAROBOT_API_ENDPOINT`) and `DATAROBOT_API_TOKEN` pair takes precedence over the config file. If the pair fails verification, the command fails with the reason (timeout, malformed endpoint, unreachable endpoint, a non-2xx status from the instance, or an invalid token; only a 401 or 403 blames the token). It never falls back to the stored profile and never starts the login flow, because that would silently run the command against a different DataRobot instance than the one requested.

[ajalon1]

👨🏽‍🚀 Nice add. I'm actually surprised we didn't handle this properly before, since auth is some of the oldest code in the CLI.

LGTM

[ajalon1]

👨🏽‍🚀 But you still deserve a 🤖 pass.

Verdict: approve — one suggestion worth considering for a follow-up. The core change is correct and well-tested: VerifyToken now returns a typed *HTTPStatusError carrying the status code, and ReportEnvCredentialsError uses it to stop blaming the token for 404/429/5xx. The one gap is that the .env leg and stored-profile leg of dr auth check / EnsureAuthenticated were left with the old "any non-200 = bad token" behavior.

---

Praise

The 401/403 vs. everything-else split is the right call, verified end-to-end. Before this PR, a 503 from the instance told the user to unset DATAROBOT_API_TOKEN — actively destructive advice that would discard working credentials. The new *HTTPStatusError type carries the status code, and ReportEnvCredentialsError only blames the token for 401/403. I confirmed the classification for 401, 403, 404, 429, and 500 via the test suite (TestReportEnvCredentialsError subtests).

The scheme check is placed in ValidateEndpoint, not SchemeHostOnly — with a comment explaining why. SchemeHostOnly is shared by dr auth set-url and dr auth export, which have different requirements. Putting the http/https-only guard in ValidateEndpoint keeps it scoped to env-credential validation. The comment ("Checked here, not in SchemeHostOnly, which set-url and export share.") makes the reasoning explicit.

🤖 Classification chain in ReportEnvCredentialsError

The function is a priority-ordered error classifier. Each check returns early; the first match wins:

err from VerifyToken
    │
    ├─ 1. context.DeadlineExceeded        → "Connection to <host> timed out"
    ├─ 2. ValidateEndpoint(endpoint) fails → "endpoint is invalid: <reason>"
    ├─ 3. endpoint has no "://"           → "endpoint is invalid: missing URL scheme"
    ├─ 4. *url.Error, Op=="parse"         → "endpoint is invalid: <parse error>"
    ├─ 5. *url.Error (transport)          → "Could not connect to <host>: <reason>"
    ├─ 6. *HTTPStatusError, ≠401 && ≠403  → "<host> answered HTTP <status>"  ← NEW
    └─ 7. default (incl. 401/403)         → "DATAROBOT_API_TOKEN is invalid or expired"

The new step 6 is inserted after the transport checks (step 5) and before the token-blaming default (step 7). This ordering is correct: a *HTTPStatusError is not a *url.Error, so errors.As(err, &urlErr) returns false for it and it falls through to step 6. A 401 or 403 fails the ≠401 && ≠403 guard and falls through to step 7 (token blamed), which is the intended behavior.

---

Suggestion (optional, not requesting a change in this PR)

S1: The .env leg and stored-profile leg still blame the token for server errors.

verifyDotenvToken in cmd/auth/check/cmd.go only checks errors.Is(err, context.DeadlineExceeded); every other non-200 falls into the else branch and prints "DATAROBOT_API_TOKEN in '.env' is invalid or expired." — the exact misattribution this PR fixes for the env-var leg.

🤖 Verified empirically with a scratch test (cleaned up after)

=== RUN   TestScratch_DotEnv503StillBlamesToken
--- PASS: TestScratch_DotEnv503StillBlamesToken (0.00s)
PASS

The test confirmed: assert.Contains(t, out, "DATAROBOT_API_TOKEN in '.env' is invalid or expired") passes, and assert.NotContains(t, out, "answered HTTP 503") passes.

The added test TestVerifyDotenvToken_StatusErrorKeepsMessage only covers 401, which is correct for 401 but doesn't expose the 503/404/429 gap. The test comment says "the typed *config.HTTPStatusError must leave its message and verdict unchanged" — so this appears to be an intentional scoping decision, but there's no comment in verifyDotenvToken itself explaining why the .env leg should behave differently from the env-var leg.

The same pattern exists in the stored-profile leg of EnsureAuthenticated (Copilot also flagged this at auth.go:253): config.GetAPIKey returns *HTTPStatusError for non-200, but EnsureAuthenticated only checks errors.Is(viperErr, context.DeadlineExceeded) and otherwise clears the token and starts the login flow. A 503 on the stored profile triggers login — the exact misbehavior the PR title claims to fix, on the path every command hits via EnsureAuthenticatedE.

If the intent is to keep these legs simple for now, a one-line comment in each would make that explicit. If the intent is to extend the fix in a follow-up, the tests should cover non-401 statuses.

---

Nits

🤖 Minor, all non-blocking

N1: TestVerifyDotenvToken_StatusErrorKeepsMessage restores os.Stdout manually, not via t.Cleanup. If verifyDotenvToken panicked between os.Stdout = w and os.Stdout = orig, stdout would stay piped for the rest of the test process. Using t.Cleanup(func() { os.Stdout = orig }) is more robust. Low risk since verifyDotenvToken is simple, but it's the pattern the rest of the test file already follows.

N2: verifyDotenvToken writes to fmt.Println (global stdout) instead of accepting an io.Writer. This is pre-existing — the PR inherits it — but it's why the test has to redirect os.Stdout with a pipe instead of passing a buffer. The sibling function checkCLICredentials already takes an io.Writer, so the pattern exists in the same file.

---

Verification notes

• Scratch test: confirmed the .env 503 behavior, then removed the scratch file.

---

How this fits with the rest

For context, the auth flow has three credential paths that call VerifyToken:

DATAROBOT_ENDPOINT + DATAROBOT_API_TOKEN (env vars)
    └─ VerifyEnvCredentials → ReportEnvCredentialsError  ← FIXED in this PR

.env file (dr auth check in a repo)
    └─ verifyDotenvToken                                   ← gap (S1)

stored profile (drconfig.yaml, every command)
    └─ EnsureAuthenticated → GetAPIKey                     ← gap (S1)

This PR fixes the env-var path. The other two paths still treat any non-200 as a token problem. The env-var path is the one where misattribution is most dangerous (the user explicitly chose an instance), so it's the right one to fix first — but the other two are worth a follow-up.

[ajalon1]

go ahead and ignore the govulncheck, that's a new version of Go available

[chasdr]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 54b5b4a. Configure here.}

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (3)

internal/auth/auth_test.go:340

• This output test checks only fragments, so duplicated text, wrong line structure, or other unexpected output would go unnoticed. Compare the full two-line status message to verify the new user-facing format.

			assert.Contains(t, buf.String(), fmt.Sprintf("answered HTTP %d", status))
			assert.Contains(t, buf.String(), "https://app.example.com")
			assert.NotContains(t, buf.String(), "unset DATAROBOT_API_TOKEN")

docs/development/authentication.md:32

• The implementation treats every status except 200 as a verification failure, including 2xx responses such as 204. Describing this as only “non-2xx” makes the documented authentication contract disagree with VerifyToken; use “non-200” here.

1. **Checks environment credentials first**: A complete `DATAROBOT_ENDPOINT` (or `DATAROBOT_API_ENDPOINT`) and `DATAROBOT_API_TOKEN` pair takes precedence over the config file. If the pair fails verification, the command fails with the reason (timeout, malformed endpoint, unreachable endpoint, a non-2xx status from the instance, or an invalid token; only a 401 or 403 blames the token). It never falls back to the stored profile and never starts the login flow, because that would silently run the command against a different DataRobot instance than the one requested.

internal/auth/auth_test.go:326

• These substring assertions do not pin the exact 401/403 wording as intended: extra or malformed output would still pass. Assert the complete rendered message so changes to line breaks or the unset instruction are caught.

This issue also appears on line 338 of the same file.

			assert.Contains(t, buf.String(), "DATAROBOT_API_TOKEN environment variable is invalid or expired")
			assert.Contains(t, buf.String(), "unset DATAROBOT_API_TOKEN")