fix(doctor): run every check instead of failing at the first problem

tea doctor printed the resolved instance, account and repository and
checked nothing; it failed with exit code 4 exactly when it was needed,
such as in a clone of an instance without an account, and with a
GITEA_TOKEN of another instance it blamed GITEA_INSTANCE_URL.

It now checks the settings files, whether the instance answers (version
and latency), whether the account's credential works and its token
scopes, whether the repository is readable (default branch), Git and
whether it asks tea for credentials, and where SSH remotes connect. Each
check shows a mark on a terminal and the command that fixes a problem;
piped, each is a tab-separated line. A GITEA_TOKEN for another instance
names both instances. doctor exits with 1 when a check fails, and --json
keeps the target fields and adds checks[].

Signed-off-by: bircni <bircni@icloud.com>

internal/app/app.go

@@ -521,7 +521,7 @@ func commandOutputFields(cmd *cobra.Command) []string {
 		}
 	case "tea":
 		if cmd.Name() == "doctor" {
-			return jsonFields[target]()
+			return jsonFields[doctorReport]()
 		}
 	case "attachment":
 		return jsonFields[services.IssueAttachment]()

internal/app/app_test.go

@@ -427,7 +427,6 @@ const (
 	branch rename
 	branch unprotect
 	branch view
-	doctor
 	issue attachment delete
 	issue attachment download
 	issue attachment edit

internal/app/doctor.go

@@ -4,27 +4,238 @@
 package app
 
 import (
+	"context"
+	"errors"
 	"fmt"
+	"strings"
+	"time"
 
 	"gitea.dev/tea/internal/clierr"
+	"gitea.dev/tea/internal/git"
+	"gitea.dev/tea/internal/services"
+	"gitea.dev/tea/internal/settings"
+	"gitea.dev/tea/internal/terminal"
 	"github.com/spf13/cobra"
 )
 
-func (a *application) doctorCommand() *cobra.Command {
+// Outcomes of a doctor check: only failed checks make tea doctor fail.
-	return &cobra.Command{Use: "doctor", Short: "Inspect resolved instance and repository context", Args: argsExact(0), RunE: func(cmd *cobra.Command, _ []string) error {
+const (
-		// Repository discovery is best-effort: doctor must also work outside a clone.
+	checkPassed  = "ok"
-		// Only discovery (usage) failures fall back; authentication errors are reported.
+	checkWarning = "warning"
-		target, err := a.resolve(cmd.Context(), true)
+	checkFailed  = "failed"
-		if clierr.ExitCode(err) == clierr.Usage && a.options.repo == "" && a.deps.Getenv("TEA_REPO") == "" {
+	checkSkipped = "skipped"
-			target, err = a.resolve(cmd.Context(), false)
+)
-		}
+
-		if err != nil {
+// doctorCheck is one diagnosis of tea doctor, with the command that fixes a problem.
-			return err
+type doctorCheck struct {
-		}
+	Name   string `json:"name"`
-		if a.options.json != "" {
+	Status string `json:"status"`
-			return a.writeJSON(cmd, target)
+	Detail string `json:"detail"`
-		}
+	Fix    string `json:"fix,omitempty"`
-		_, err = fmt.Fprintf(cmd.OutOrStdout(), "instance\t%s\t%s\naccount\t%s\t%s\nrepository\t%s\t%s\n", target.Host, target.HostSource, target.User, target.UserSource, target.Repo, target.RepoSource)
+}
-		return err
+
-	}}
+// doctorReport is the instance, account and repository that commands select here, and the checks
+// of each.
+type doctorReport struct {
+	Host       string        `json:"host"`
+	User       string        `json:"user"`
+	Repo       string        `json:"repo"`
+	HostSource string        `json:"host_source"`
+	UserSource string        `json:"user_source"`
+	RepoSource string        `json:"repo_source"`
+	Checks     []doctorCheck `json:"checks"`
+}
+
+// doctorTimeout bounds each request of a check.
+const doctorTimeout = 10 * time.Second
+
+func (a *application) doctorCommand() *cobra.Command {
+	return &cobra.Command{
+		Use: "doctor", Short: "Check tea's setup for this directory", Args: argsExact(0),
+		Long: `Check the setup that commands in this directory use: the settings files, the instance
+and whether it answers, the account and its credential, the repository, and Git. Each
+problem comes with the command that fixes it. tea doctor fails when a check fails.`,
+		Example: "$ tea doctor\n# Check another repository and instance\n$ tea doctor -R gitea.com/gitea/tea\n$ tea doctor --jq '.checks[] | select(.status == \"failed\")'",
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			report := a.diagnose(cmd.Context())
+			failed := 0
+			for _, check := range report.Checks {
+				if check.Status == checkFailed {
+					failed++
+				}
+			}
+			var err error
+			if a.options.json != "" {
+				err = a.writeJSON(cmd, report)
+			} else {
+				err = a.writeHuman(cmd, doctorTable(report.Checks, a.display()))
+			}
+			if err == nil && failed > 0 {
+				err = fmt.Errorf("%d of %d checks failed", failed, len(report.Checks))
+			}
+			return err
+		},
+	}
+}
+
+// diagnose resolves the target like other commands, but reports each problem instead of stopping
+// at the first.
+func (a *application) diagnose(ctx context.Context) doctorReport {
+	var report doctorReport
+	add := func(check doctorCheck) { report.Checks = append(report.Checks, check) }
+	snapshot, err := a.deps.Settings.Load()
+	if err != nil {
+		add(doctorCheck{"Config", checkFailed, err.Error(), "repair or move the file; tea config path prints both files"})
+		add(a.gitCheck(ctx, ""))
+		return report
+	}
+	add(doctorCheck{"Config", checkPassed, "config.yml and hosts.yml in " + a.deps.Settings.Dir, ""})
+	t, err := a.resolveTarget(ctx, true, false)
+	repoErr := err
+	if err != nil && clierr.ExitCode(err) == clierr.Usage && a.options.repo == "" && a.deps.Getenv("TEA_REPO") == "" {
+		t, err = a.resolveTarget(ctx, false, false)
+	}
+	report.Host, report.User, report.Repo = t.Host, t.User, t.Repo
+	report.HostSource, report.UserSource, report.RepoSource = t.HostSource, t.UserSource, t.RepoSource
+	if t.Host == "" {
+		add(doctorCheck{"Instance", checkFailed, Explain(ctx, a.deps, err).Error(), "tea auth login --host URL, or pass --host"})
+		add(a.gitCheck(ctx, ""))
+		return report
+	}
+	instance := a.instanceCheck(ctx, t)
+	add(instance)
+	authenticated, accountErr := a.resolveTarget(ctx, t.Repo != "", true)
+	switch {
+	case instance.Status == checkFailed:
+		accountErr = errors.New("instance does not answer")
+		detail := "not checked while the instance does not answer"
+		if t.User != "" {
+			detail = t.User + ": " + detail
+		}
+		add(doctorCheck{"Account", checkSkipped, detail, ""})
+	case accountErr == nil:
+		report.User, report.UserSource = authenticated.User, authenticated.UserSource
+		fallthrough
+	default:
+		add(a.accountCheck(ctx, t, snapshot, authenticated, accountErr))
+	}
+	switch {
+	case t.Repo == "":
+		detail := "none: not in a clone of a repository on " + t.Host
+		if repoErr != nil && !strings.HasPrefix(repoErr.Error(), errRepositoryRequired.Error()) {
+			detail = repoErr.Error()
+		}
+		add(doctorCheck{"Repository", checkSkipped, detail, ""})
+	case accountErr != nil:
+		add(doctorCheck{"Repository", checkSkipped, t.Repo + " (" + describeSource(t.RepoSource) + "); not checked without a working account", ""})
+	default:
+		add(a.repositoryCheck(ctx, authenticated))
+	}
+	add(a.gitCheck(ctx, t.Host))
+	if check, ok := sshCheck(t); ok {
+		add(check)
+	}
+	return report
+}
+
+// instanceCheck asks the instance for its version.
+func (a *application) instanceCheck(ctx context.Context, t target) doctorCheck {
+	ctx, cancel := context.WithTimeout(ctx, doctorTimeout)
+	defer cancel()
+	start := time.Now()
+	version, err := services.ServerVersion(ctx, a.deps.Client(t.Host, "", t.host.Insecure))
+	where := t.Host + " (" + describeSource(t.HostSource) + ")"
+	if err != nil {
+		return doctorCheck{"Instance", checkFailed, where + ": " + Explain(ctx, a.deps, err).Error(), "check --host, the instance URL and your network"}
+	}
+	return doctorCheck{"Instance", checkPassed, fmt.Sprintf("%s · Gitea %s · %s", where, version, time.Since(start).Round(time.Millisecond)), ""}
+}
+
+// accountCheck reports whether the account authenticates, and the scopes of its token.
+func (a *application) accountCheck(ctx context.Context, t target, snapshot settings.Snapshot, authenticated target, err error) doctorCheck {
+	login := "tea auth login --host " + t.Host
+	if a.deps.Getenv("GITEA_TOKEN") != "" && err != nil {
+		instance, resolveErr := settings.ResolveHost(a.tokenInstance(), snapshot.Hosts.Hosts)
+		switch {
+		case a.tokenInstance() == "" || resolveErr != nil:
+			return doctorCheck{"Account", checkFailed, "GITEA_TOKEN is set, but neither GITEA_INSTANCE_URL nor TEA_HOST names its instance", "export GITEA_INSTANCE_URL=" + t.Host}
+		case instance != t.Host:
+			return doctorCheck{"Account", checkFailed, "GITEA_TOKEN belongs to " + instance + ", but the instance here is " + t.Host + " (" + describeSource(t.HostSource) + ")", "pass --host " + instance + " or -R a repository there, or unset GITEA_TOKEN"}
+		}
+	}
+	if err != nil {
+		detail := Explain(ctx, a.deps, err).Error()
+		if strings.Contains(detail, login) {
+			login = ""
+		}
+		return doctorCheck{"Account", checkFailed, detail, login}
+	}
+	ctx, cancel := context.WithTimeout(ctx, doctorTimeout)
+	defer cancel()
+	user, err := services.CurrentAccount(ctx, authenticated.client)
+	if err != nil {
+		return doctorCheck{"Account", checkFailed, authenticated.User + ": " + Explain(ctx, a.deps, err).Error(), login}
+	}
+	detail := user + " (" + describeSource(authenticated.UserSource) + ") · credential valid"
+	if scopes, err := services.TokenScopes(ctx, authenticated.client); err == nil && len(scopes) > 0 {
+		detail += " · scopes: " + strings.Join(scopes, ", ")
+	}
+	return doctorCheck{"Account", checkPassed, detail, ""}
+}
+
+// repositoryCheck reports whether the account can read the repository.
+func (a *application) repositoryCheck(ctx context.Context, t target) doctorCheck {
+	ctx, cancel := context.WithTimeout(ctx, doctorTimeout)
+	defer cancel()
+	where := t.Repo + " (" + describeSource(t.RepoSource) + ")"
+	repo, err := services.Repositories{Client: t.client}.View(ctx, t.Repo)
+	if err != nil {
+		return doctorCheck{"Repository", checkFailed, where + ": " + Explain(ctx, a.deps, err).Error(), "check the remote URL, or pass -R OWNER/REPO"}
+	}
+	return doctorCheck{"Repository", checkPassed, where + " · default branch " + repo.DefaultBranch, ""}
+}
+
+// gitCheck reports the installed Git and whether it asks tea for the credentials of instance.
+func (a *application) gitCheck(ctx context.Context, instance string) doctorCheck {
+	version, err := a.gitProbe().Output(ctx, a.options.cwd, false, "--version")
+	switch {
+	case err != nil:
+		return doctorCheck{"Git", checkFailed, "Git is not installed or cannot run", "install Git"}
+	case instance == "":
+		return doctorCheck{"Git", checkPassed, version, ""}
+	}
+	if scope := git.HelperScope(ctx, a.gitProbe(), a.options.cwd, instance); scope != "" {
+		return doctorCheck{"Git", checkPassed, version + " · tea is the credential helper (" + scope + ")", ""}
+	}
+	return doctorCheck{"Git", checkWarning, version + " · credential helper not configured for " + instance, "tea auth setup-git, if pushes over HTTPS should use this account"}
+}
+
+// sshCheck reports where SSH remotes connect when the instance uses SSH.
+func sshCheck(t target) (doctorCheck, bool) {
+	switch {
+	case t.host.SSHHost != "":
+		return doctorCheck{"SSH", checkPassed, "remotes connect to " + t.host.SSHHost + " (ssh_host)", ""}, true
+	case t.gitProtocol == "ssh":
+		return doctorCheck{"SSH", checkWarning, "remotes connect to the web host on port 22", "tea config set --host " + t.Host + " ssh_host HOST:PORT, if SSH listens elsewhere"}, true
+	}
+	return doctorCheck{}, false
+}
+
+// doctorTable lists the checks: on a terminal with an icon per outcome and the fix under a problem,
+// otherwise as tab-separated status, name, detail and fix.
+func doctorTable(checks []doctorCheck, d terminal.Display) string {
+	icons := map[string]terminal.Cell{
+		checkPassed:  {Text: "✓", Color: "green"},
+		checkWarning: {Text: "!", Color: "yellow"},
+		checkFailed:  {Text: "X", Color: "red"},
+		checkSkipped: {Text: "-", Muted: true},
+	}
+	rows := make([][]terminal.Cell, 0, len(checks))
+	for _, check := range checks {
+		rows = append(rows, []terminal.Cell{icons[check.Status], {Text: check.Status}, {Text: check.Name, Bold: true}, {Text: check.Detail}, {Text: check.Fix}})
+		if d.TTY && check.Fix != "" && check.Status != checkPassed {
+			rows = append(rows, []terminal.Cell{{}, {}, {}, {Text: "→ " + check.Fix, Muted: true}})
+		}
+	}
+	return terminal.Table([]terminal.Column{{TTYOnly: true}, {PipedOnly: true}, {}, {}, {PipedOnly: true}}, rows, d)
 }

internal/app/doctor_test.go

@@ -5,17 +5,44 @@ package app
 
 import (
 	"context"
+	"encoding/json"
 	"net/http"
+	"strings"
 	"testing"
 
+	"gitea.dev/tea/internal/auth"
 	"gitea.dev/tea/internal/clierr"
+	"gitea.dev/tea/internal/settings"
 	"github.com/stretchr/testify/require"
 )
 
+// doctorServer is an instance that answers the requests of tea doctor for alice/project.
+func doctorServer(t *testing.T) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/version":
+			_, _ = w.Write([]byte(`{"version":"28.1.0"}`))
+		case "/api/v1/token":
+			_, _ = w.Write([]byte(`{"scopes":["all"]}`))
+		case "/api/v1/repos/alice/project":
+			_, _ = w.Write([]byte(`{"full_name":"alice/project","default_branch":"main"}`))
+		default:
+			t.Errorf("unexpected request %s", r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
+		}
+	})
+}
+
+// doctorGit is a Git checkout of alice/project at url, without tea as credential helper.
+func doctorGit(url string) *fakeGit {
+	return &fakeGit{results: map[string]string{"--version": "git version 2.50.1", "remote": "origin", "remote get-url origin": url + "/alice/project.git"}}
+}
+
 func TestEnvironmentCredentialReportsActualAccount(t *testing.T) {
-	server := newAuthenticatedServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { t.Error("unexpected request") }))
+	server := newAuthenticatedServer(doctorServer(t))
 	defer server.Close()
 	deps, out, _ := testDeps(t)
+	deps.Git = &fakeGit{results: map[string]string{"--version": "git version 2.50.1"}}
 	deps.Getenv = serverEnv(server.URL, map[string]string{"TEA_USER": "ALICE"})
 	require.NoError(t, Execute(context.Background(), deps, []string{"doctor", "--json"}))
 	require.Contains(t, out.String(), `"user":"alice"`)
@@ -23,13 +50,90 @@ func TestEnvironmentCredentialReportsActualAccount(t *testing.T) {
 }
 
 func TestDoctorDiscoversRepositoryBestEffort(t *testing.T) {
-	server := newAuthenticatedServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { t.Error("unexpected request") }))
+	server := newAuthenticatedServer(doctorServer(t))
 	defer server.Close()
 	deps, out, _ := serverDeps(t, server.URL)
-	deps.Git = &fakeGit{results: map[string]string{"remote": "origin", "remote get-url origin": server.URL + "/alice/project.git"}}
+	deps.Git = doctorGit(server.URL)
 	require.NoError(t, Execute(context.Background(), deps, []string{"doctor", "--json=repo,repo_source"}))
 	require.JSONEq(t, `{"repo":"alice/project","repo_source":"remote origin"}`, out.String())
-	out.Reset()
+}
-	deps.Getenv = envFunc(map[string]string{"TEA_HOST": server.URL})
+
-	require.Equal(t, clierr.Authentication, clierr.ExitCode(Execute(context.Background(), deps, []string{"doctor"})))
+func TestDoctorReportsEveryCheckInsteadOfFailingFirst(t *testing.T) {
+	server := newAuthenticatedServer(doctorServer(t))
+	defer server.Close()
+	origin := `(git remote "origin")`
+	for _, tc := range []struct {
+		name     string
+		env      map[string]string
+		args     []string
+		statuses string
+		account  string
+	}{
+		{
+			name: "environment token", env: map[string]string{"TEA_HOST": server.URL, "GITEA_TOKEN": "token"},
+			statuses: "Config ok, Instance ok, Account ok, Repository ok, Git warning",
+			account:  "alice (GITEA_TOKEN authenticated account) · credential valid · scopes: all",
+		},
+		{
+			name: "no account for the instance of the remote", env: map[string]string{"TEA_HOST": server.URL},
+			statuses: "Config ok, Instance ok, Account failed, Repository skipped, Git warning",
+			account:  "no account for " + server.URL + " (instance from " + strings.Trim(origin, "()") + "); run tea auth login --host " + server.URL + ", or use -R or --host to target another instance",
+		},
+		{
+			name: "token of another instance", env: map[string]string{"TEA_HOST": "https://other.example.org", "GITEA_TOKEN": "token"},
+			statuses: "Config ok, Instance ok, Account failed, Repository skipped, Git warning",
+			account:  "GITEA_TOKEN belongs to https://other.example.org, but the instance here is " + server.URL + " " + origin,
+		},
+		{
+			name: "instance that does not answer", args: []string{"--host", "http://127.0.0.1:1", "-R", "alice/project"},
+			statuses: "Config ok, Instance failed, Account skipped, Repository skipped, Git warning",
+			account:  "not checked while the instance does not answer",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			deps, out, _ := testDeps(t)
+			deps.Git = doctorGit(server.URL)
+			deps.Getenv = envFunc(tc.env)
+			err := Execute(context.Background(), deps, append([]string{"doctor", "--json=checks"}, tc.args...))
+			var report doctorReport
+			require.NoError(t, json.Unmarshal(out.Bytes(), &report), out.String())
+			statuses := make([]string, len(report.Checks))
+			for i, check := range report.Checks {
+				statuses[i] = check.Name + " " + check.Status
+				if check.Name == "Account" {
+					require.Equal(t, tc.account, check.Detail)
+				}
+			}
+			require.Equal(t, tc.statuses, strings.Join(statuses, ", "))
+			if strings.Contains(tc.statuses, "failed") {
+				require.Equal(t, clierr.Failure, clierr.ExitCode(err))
+				require.EqualError(t, err, "1 of 5 checks failed")
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestDoctorShowsChecksOnATerminal(t *testing.T) {
+	server := newAuthenticatedServer(doctorServer(t))
+	defer server.Close()
+	deps, out, _ := testDeps(t)
+	deps.Streams.OutTTY = true
+	deps.Git = doctorGit(server.URL)
+	configureHost(t, deps, server.URL, settings.Host{User: "alice", Users: tokenAccounts(settings.StoreFile, "alice"), SSHHost: "ssh.example.org:2222"})
+	require.NoError(t, deps.Credentials.Save(server.URL, "alice", settings.StoreFile, auth.Credential{AccessToken: "alice-token"}))
+	require.NoError(t, Execute(context.Background(), deps, []string{"doctor"}))
+	lines := withoutANSI(out.String())
+	for _, want := range []string{
+		"✓  Config      config.yml and hosts.yml in " + deps.Settings.Dir + "\n",
+		"✓  Account     alice (active instance account) · credential valid · scopes: all\n",
+		"✓  Repository  alice/project (git remote \"origin\") · default branch main\n",
+		"!  Git         git version 2.50.1 · credential helper not configured for " + server.URL + "\n" +
+			"               → tea auth setup-git, if pushes over HTTPS should use this account\n",
+		"✓  SSH         remotes connect to ssh.example.org:2222 (ssh_host)\n",
+	} {
+		require.Contains(t, lines, want)
+	}
+	require.Regexp(t, `✓  Instance    `+server.URL+` \(git remote "origin"\) · Gitea 28.1.0 · \d+ms\n`, lines)
 }