From 7434e99fc1c45c216452b725b20927a2a8386fac Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Wed, 3 Jun 2026 16:36:27 +0530 Subject: [PATCH 1/2] feat(cli): enrich ao doctor --- backend/internal/cli/doctor.go | 299 +++++++++++++++++++++++--- backend/internal/cli/doctor_test.go | 315 ++++++++++++++++++++++++++-- backend/internal/cli/e2e_test.go | 5 +- 3 files changed, 572 insertions(+), 47 deletions(-) diff --git a/backend/internal/cli/doctor.go b/backend/internal/cli/doctor.go index 02cd671c6..9d257a7a7 100644 --- a/backend/internal/cli/doctor.go +++ b/backend/internal/cli/doctor.go @@ -2,11 +2,15 @@ package cli import ( "context" + "encoding/json" "errors" "fmt" "io/fs" + "net/http" "os" "path/filepath" + "regexp" + "strings" "github.com/spf13/cobra" @@ -24,6 +28,7 @@ const ( type doctorCheck struct { Level doctorLevel `json:"level"` + Section string `json:"section,omitempty"` Name string `json:"name"` Message string `json:"message"` } @@ -34,6 +39,30 @@ type doctorReport struct { Checks []doctorCheck `json:"checks"` } +const ( + doctorSectionCore = "Core" + doctorSectionTools = "Tools" + doctorSectionAgents = "Agent harnesses" + doctorSectionGitHub = "GitHub" + minGitVersion = "2.25.0" + githubDoctorUserAgent = "ao-agent-orchestrator/doctor" +) + +var doctorGitHubRESTBase = "https://api.github.com" + +type harnessProbe struct { + Name string + BinaryName string + VersionArg string +} + +var doctorHarnesses = []harnessProbe{ + {Name: "claude-code", BinaryName: "claude", VersionArg: "--version"}, + {Name: "codex", BinaryName: "codex", VersionArg: "--version"}, + {Name: "aider", BinaryName: "aider", VersionArg: "--version"}, + {Name: "opencode", BinaryName: "opencode", VersionArg: "version"}, +} + func newDoctorCommand(ctx *commandContext) *cobra.Command { var asJSON bool cmd := &cobra.Command{ @@ -56,10 +85,8 @@ func newDoctorCommand(ctx *commandContext) *cobra.Command { return err } } else { - for _, check := range checks { - if _, err := fmt.Fprintf(cmd.OutOrStdout(), "%s %s: %s\n", check.Level, check.Name, check.Message); err != nil { - return err - } + if err := writeDoctorText(cmd, checks); err != nil { + return err } } @@ -73,29 +100,53 @@ func newDoctorCommand(ctx *commandContext) *cobra.Command { return cmd } +func writeDoctorText(cmd *cobra.Command, checks []doctorCheck) error { + var lastSection string + for _, check := range checks { + if check.Section != "" && check.Section != lastSection { + if lastSection != "" { + if _, err := fmt.Fprintln(cmd.OutOrStdout()); err != nil { + return err + } + } + if _, err := fmt.Fprintf(cmd.OutOrStdout(), "%s:\n", check.Section); err != nil { + return err + } + lastSection = check.Section + } + if _, err := fmt.Fprintf(cmd.OutOrStdout(), "%s %s: %s\n", check.Level, check.Name, check.Message); err != nil { + return err + } + } + return nil +} + func (c *commandContext) runDoctor(ctx context.Context) []doctorCheck { checks := []doctorCheck{} cfg, err := config.Load() if err != nil { - return append(checks, doctorCheck{Level: doctorFail, Name: "config", Message: err.Error()}) + return append(checks, doctorCheck{Level: doctorFail, Section: doctorSectionCore, Name: "config", Message: err.Error()}) } checks = append(checks, doctorCheck{ - Level: doctorPass, Name: "config", + Level: doctorPass, Section: doctorSectionCore, Name: "config", Message: fmt.Sprintf("runFile=%s dataDir=%s port=%d", cfg.RunFilePath, cfg.DataDir, cfg.Port), }) if err := os.MkdirAll(cfg.DataDir, 0o750); err != nil { - checks = append(checks, doctorCheck{Level: doctorFail, Name: "data-dir", Message: err.Error()}) + checks = append(checks, doctorCheck{Level: doctorFail, Section: doctorSectionCore, Name: "data-dir", Message: err.Error()}) } else { - checks = append(checks, doctorCheck{Level: doctorPass, Name: "data-dir", Message: cfg.DataDir}) + checks = append(checks, + doctorCheck{Level: doctorPass, Section: doctorSectionCore, Name: "data-dir", Message: cfg.DataDir}, + checkDataDirWritable(cfg.DataDir), + ) } checks = append(checks, checkStore(cfg.DataDir)) st, err := c.inspectDaemon(ctx) if err != nil { - checks = append(checks, doctorCheck{Level: doctorFail, Name: "daemon", Message: err.Error()}) + checks = append(checks, doctorCheck{Level: doctorFail, Section: doctorSectionCore, Name: "daemon", Message: err.Error()}) } else { level := doctorPass switch st.State { @@ -111,13 +162,17 @@ func (c *commandContext) runDoctor(ctx context.Context) []doctorCheck { if st.Error != "" { msg += " (" + st.Error + ")" } - checks = append(checks, doctorCheck{Level: level, Name: "daemon", Message: msg}) + checks = append(checks, doctorCheck{Level: level, Section: doctorSectionCore, Name: "daemon", Message: msg}) } checks = append(checks, - c.checkTool("git", true), + c.checkGit(ctx), c.checkZellij(ctx), ) + for _, harness := range doctorHarnesses { + checks = append(checks, c.checkHarness(ctx, harness)) + } + checks = append(checks, c.checkGitHubToken(ctx)) return checks } @@ -133,44 +188,236 @@ func checkStore(dataDir string) doctorCheck { switch { case err == nil: return doctorCheck{ - Level: doctorPass, Name: "sqlite", + Level: doctorPass, Section: doctorSectionCore, Name: "sqlite", Message: fmt.Sprintf("%s (%d bytes); migrations are applied by the daemon at startup", dbPath, info.Size()), } case errors.Is(err, fs.ErrNotExist): return doctorCheck{ - Level: doctorWarn, Name: "sqlite", + Level: doctorWarn, Section: doctorSectionCore, Name: "sqlite", Message: "database not created yet; run `ao start` to initialize and migrate it", } default: - return doctorCheck{Level: doctorFail, Name: "sqlite", Message: err.Error()} + return doctorCheck{Level: doctorFail, Section: doctorSectionCore, Name: "sqlite", Message: err.Error()} + } +} + +func checkDataDirWritable(dataDir string) doctorCheck { + f, err := os.CreateTemp(dataDir, ".ao-doctor-write-*") + if err != nil { + return doctorCheck{Level: doctorFail, Section: doctorSectionCore, Name: "data-dir-write", Message: err.Error()} + } + name := f.Name() + if _, err := f.WriteString("ok\n"); err != nil { + _ = f.Close() + _ = os.Remove(name) + return doctorCheck{Level: doctorFail, Section: doctorSectionCore, Name: "data-dir-write", Message: err.Error()} } + if err := f.Close(); err != nil { + _ = os.Remove(name) + return doctorCheck{Level: doctorFail, Section: doctorSectionCore, Name: "data-dir-write", Message: err.Error()} + } + if err := os.Remove(name); err != nil { + return doctorCheck{Level: doctorWarn, Section: doctorSectionCore, Name: "data-dir-write", Message: fmt.Sprintf("write probe succeeded but cleanup failed: %v", err)} + } + return doctorCheck{Level: doctorPass, Section: doctorSectionCore, Name: "data-dir-write", Message: "write probe succeeded"} +} + +func (c *commandContext) checkGit(ctx context.Context) doctorCheck { + path, err := c.deps.LookPath("git") + if err != nil || path == "" { + return doctorCheck{Level: doctorFail, Section: doctorSectionTools, Name: "git", Message: "not found in PATH"} + } + reqCtx, cancel := context.WithTimeout(ctx, probeTimeout) + defer cancel() + out, err := c.deps.CommandOutput(reqCtx, path, "--version") + if err != nil { + return doctorCheck{Level: doctorFail, Section: doctorSectionTools, Name: "git", Message: fmt.Sprintf("%s: %v", path, err)} + } + version, err := parseGitVersion(string(out)) + if err != nil { + return doctorCheck{Level: doctorWarn, Section: doctorSectionTools, Name: "git", Message: fmt.Sprintf("%s (version unknown: %s)", path, firstOutputLine(out))} + } + if compareDottedVersion(version, minGitVersion) < 0 { + return doctorCheck{Level: doctorWarn, Section: doctorSectionTools, Name: "git", Message: fmt.Sprintf("%s (version %s; AO expects >= %s for worktrees)", path, version, minGitVersion)} + } + return doctorCheck{Level: doctorPass, Section: doctorSectionTools, Name: "git", Message: fmt.Sprintf("%s (version %s; supports worktrees)", path, version)} } func (c *commandContext) checkZellij(ctx context.Context) doctorCheck { path, err := c.deps.LookPath("zellij") - if err != nil { - return doctorCheck{Level: doctorWarn, Name: "zellij", Message: "not found in PATH"} + if err != nil || path == "" { + return doctorCheck{Level: doctorWarn, Section: doctorSectionTools, Name: "zellij", Message: "not found in PATH"} } reqCtx, cancel := context.WithTimeout(ctx, probeTimeout) defer cancel() out, err := c.deps.CommandOutput(reqCtx, path, "--version") if err != nil { - return doctorCheck{Level: doctorFail, Name: "zellij", Message: fmt.Sprintf("%s: %v", path, err)} + return doctorCheck{Level: doctorFail, Section: doctorSectionTools, Name: "zellij", Message: fmt.Sprintf("%s: %v", path, err)} } version, err := zellij.CheckVersionOutput(string(out)) if err != nil { - return doctorCheck{Level: doctorFail, Name: "zellij", Message: fmt.Sprintf("%s: %v", path, err)} + return doctorCheck{Level: doctorFail, Section: doctorSectionTools, Name: "zellij", Message: fmt.Sprintf("%s: %v", path, err)} } - return doctorCheck{Level: doctorPass, Name: "zellij", Message: fmt.Sprintf("%s (%s)", path, version)} + return doctorCheck{Level: doctorPass, Section: doctorSectionTools, Name: "zellij", Message: fmt.Sprintf("%s (version %s; require >= %s)", path, version, zellij.RequiredVersion())} } -func (c *commandContext) checkTool(name string, required bool) doctorCheck { - path, err := c.deps.LookPath(name) - if err == nil { - return doctorCheck{Level: doctorPass, Name: name, Message: path} +func (c *commandContext) checkHarness(ctx context.Context, harness harnessProbe) doctorCheck { + path, err := c.deps.LookPath(harness.BinaryName) + if err != nil || path == "" { + return doctorCheck{ + Level: doctorWarn, Section: doctorSectionAgents, Name: harness.Name, + Message: fmt.Sprintf("%s not found in PATH", harness.BinaryName), + } + } + if harness.VersionArg == "" { + return doctorCheck{Level: doctorPass, Section: doctorSectionAgents, Name: harness.Name, Message: fmt.Sprintf("%s resolves to %s", harness.BinaryName, path)} + } + reqCtx, cancel := context.WithTimeout(ctx, probeTimeout) + defer cancel() + out, err := c.deps.CommandOutput(reqCtx, path, harness.VersionArg) + if err != nil { + return doctorCheck{ + Level: doctorWarn, Section: doctorSectionAgents, Name: harness.Name, + Message: fmt.Sprintf("%s resolves to %s, but `%s %s` failed: %v", harness.BinaryName, path, harness.BinaryName, harness.VersionArg, err), + } + } + version := firstOutputLine(out) + if version == "" { + version = "version output was empty" } - if required { - return doctorCheck{Level: doctorFail, Name: name, Message: "not found in PATH"} + return doctorCheck{Level: doctorPass, Section: doctorSectionAgents, Name: harness.Name, Message: fmt.Sprintf("%s resolves to %s (%s)", harness.BinaryName, path, version)} +} + +func (c *commandContext) checkGitHubToken(ctx context.Context) doctorCheck { + token, source, err := c.githubToken(ctx) + if err != nil { + return doctorCheck{Level: doctorWarn, Section: doctorSectionGitHub, Name: "github-token", Message: err.Error()} + } + + reqCtx, cancel := context.WithTimeout(ctx, probeTimeout) + defer cancel() + req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, strings.TrimRight(doctorGitHubRESTBase, "/")+"/user", http.NoBody) + if err != nil { + return doctorCheck{Level: doctorFail, Section: doctorSectionGitHub, Name: "github-token", Message: err.Error()} + } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + req.Header.Set("User-Agent", githubDoctorUserAgent) + req.Header.Set("Authorization", "Bearer "+token) + resp, err := c.deps.HTTPClient.Do(req) + if err != nil { + return doctorCheck{Level: doctorFail, Section: doctorSectionGitHub, Name: "github-token", Message: fmt.Sprintf("%s token validation failed: %v", source, err)} + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { + return doctorCheck{Level: doctorFail, Section: doctorSectionGitHub, Name: "github-token", Message: fmt.Sprintf("%s token rejected by GitHub (HTTP %d)", source, resp.StatusCode)} + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return doctorCheck{Level: doctorWarn, Section: doctorSectionGitHub, Name: "github-token", Message: fmt.Sprintf("%s token probe returned HTTP %d", source, resp.StatusCode)} + } + + var user struct { + Login string `json:"login"` + } + if err := json.NewDecoder(resp.Body).Decode(&user); err != nil { + return doctorCheck{Level: doctorFail, Section: doctorSectionGitHub, Name: "github-token", Message: fmt.Sprintf("%s token probe decode failed: %v", source, err)} + } + login := user.Login + if login == "" { + login = "unknown user" + } + scopes := strings.TrimSpace(resp.Header.Get("X-OAuth-Scopes")) + scopeMsg := "scopes unavailable" + if scopes != "" { + scopeMsg = "scopes: " + scopes + } + return doctorCheck{Level: doctorPass, Section: doctorSectionGitHub, Name: "github-token", Message: fmt.Sprintf("%s token valid for %s (%s)", source, login, scopeMsg)} +} + +func (c *commandContext) githubToken(ctx context.Context) (token, source string, err error) { + for _, name := range []string{"AO_GITHUB_TOKEN", "GITHUB_TOKEN"} { + if v := strings.TrimSpace(os.Getenv(name)); v != "" { + return v, name, nil + } + } + path, lookErr := c.deps.LookPath("gh") + if lookErr != nil || path == "" { + return "", "", errors.New("no GitHub token found (set AO_GITHUB_TOKEN/GITHUB_TOKEN or run `gh auth login`)") + } + reqCtx, cancel := context.WithTimeout(ctx, probeTimeout) + defer cancel() + out, cmdErr := c.deps.CommandOutput(reqCtx, path, "auth", "token") + if cmdErr != nil { + return "", "", fmt.Errorf("gh is installed but no token was available (`gh auth token` failed: %w)", cmdErr) + } + token = strings.TrimSpace(string(out)) + if token == "" { + return "", "", errors.New("gh is installed but returned an empty auth token") + } + return token, "gh", nil +} + +var ( + ansiRE = regexp.MustCompile(`\x1b\[[0-9;]*[A-Za-z]`) + gitVersionRE = regexp.MustCompile(`(?i)\bgit version\s+(\d+(?:\.\d+){1,3})`) +) + +func parseGitVersion(out string) (string, error) { + clean := ansiRE.ReplaceAllString(out, "") + m := gitVersionRE.FindStringSubmatch(clean) + if len(m) < 2 { + return "", fmt.Errorf("parse git version from %q", strings.TrimSpace(clean)) + } + return m[1], nil +} + +func firstOutputLine(out []byte) string { + clean := strings.TrimSpace(ansiRE.ReplaceAllString(string(out), "")) + if clean == "" { + return "" + } + line := strings.SplitN(clean, "\n", 2)[0] + return strings.TrimSpace(line) +} + +func compareDottedVersion(a, b string) int { + ap := dottedVersionParts(a) + bp := dottedVersionParts(b) + maxLen := len(ap) + if len(bp) > maxLen { + maxLen = len(bp) + } + for i := 0; i < maxLen; i++ { + var av, bv int + if i < len(ap) { + av = ap[i] + } + if i < len(bp) { + bv = bp[i] + } + switch { + case av < bv: + return -1 + case av > bv: + return 1 + } + } + return 0 +} + +func dottedVersionParts(s string) []int { + raw := strings.Split(s, ".") + parts := make([]int, 0, len(raw)) + for _, part := range raw { + n := 0 + for _, ch := range part { + if ch < '0' || ch > '9' { + break + } + n = n*10 + int(ch-'0') + } + parts = append(parts, n) } - return doctorCheck{Level: doctorWarn, Name: name, Message: "not found in PATH"} + return parts } diff --git a/backend/internal/cli/doctor_test.go b/backend/internal/cli/doctor_test.go index 14dfcb2c5..d495ff715 100644 --- a/backend/internal/cli/doctor_test.go +++ b/backend/internal/cli/doctor_test.go @@ -2,23 +2,69 @@ package cli import ( "context" + "encoding/json" "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" "strings" "testing" ) +func TestDoctorChecksGitVersion(t *testing.T) { + setConfigEnv(t) + c := doctorContext(t, map[string]string{"git": "/bin/git"}, func(_ context.Context, name string, args ...string) ([]byte, error) { + if name != "/bin/git" || len(args) != 1 || args[0] != "--version" { + t.Fatalf("unexpected command: %s %v", name, args) + } + return []byte("git version 2.43.0\n"), nil + }) + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "git") + if check.Level != doctorPass || !strings.Contains(check.Message, "2.43.0") || !strings.Contains(check.Message, "supports worktrees") { + t.Fatalf("git check = %+v, want PASS with version", check) + } +} + +func TestDoctorWarnsOnUnsupportedGitVersion(t *testing.T) { + setConfigEnv(t) + c := doctorContext(t, map[string]string{"git": "/bin/git"}, func(context.Context, string, ...string) ([]byte, error) { + return []byte("git version 2.24.9\n"), nil + }) + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "git") + if check.Level != doctorWarn || !strings.Contains(check.Message, ">= 2.25.0") { + t.Fatalf("git check = %+v, want WARN with minimum version", check) + } +} + +func TestDoctorFailsWhenGitMissing(t *testing.T) { + setConfigEnv(t) + c := doctorContext(t, map[string]string{}, nil) + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "git") + if check.Level != doctorFail { + t.Fatalf("git check = %+v, want FAIL", check) + } +} + func TestDoctorChecksZellijVersion(t *testing.T) { setConfigEnv(t) - cmdPath := map[string]string{"git": "/bin/git", "zellij": "/bin/zellij"} - c := &commandContext{deps: Deps{ - LookPath: func(name string) (string, error) { return cmdPath[name], nil }, - CommandOutput: func(_ context.Context, name string, args ...string) ([]byte, error) { - if name != "/bin/zellij" || len(args) != 1 || args[0] != "--version" { - t.Fatalf("unexpected command: %s %v", name, args) + c := doctorContext(t, map[string]string{"git": "/bin/git", "zellij": "/bin/zellij"}, func(_ context.Context, name string, args ...string) ([]byte, error) { + switch name { + case "/bin/git": + return []byte("git version 2.43.0\n"), nil + case "/bin/zellij": + if len(args) != 1 || args[0] != "--version" { + t.Fatalf("unexpected zellij command: %s %v", name, args) } return []byte("zellij 0.44.3\n"), nil - }, - }.withDefaults()} + default: + t.Fatalf("unexpected command: %s %v", name, args) + return nil, nil + } + }) check := findDoctorCheck(t, c.runDoctor(context.Background()), "zellij") if check.Level != doctorPass || !strings.Contains(check.Message, "0.44.3") { @@ -28,13 +74,12 @@ func TestDoctorChecksZellijVersion(t *testing.T) { func TestDoctorFailsUnsupportedZellijVersion(t *testing.T) { setConfigEnv(t) - cmdPath := map[string]string{"git": "/bin/git", "zellij": "/bin/zellij"} - c := &commandContext{deps: Deps{ - LookPath: func(name string) (string, error) { return cmdPath[name], nil }, - CommandOutput: func(context.Context, string, ...string) ([]byte, error) { - return []byte("zellij 0.44.2\n"), nil - }, - }.withDefaults()} + c := doctorContext(t, map[string]string{"git": "/bin/git", "zellij": "/bin/zellij"}, func(_ context.Context, name string, _ ...string) ([]byte, error) { + if name == "/bin/git" { + return []byte("git version 2.43.0\n"), nil + } + return []byte("zellij 0.44.2\n"), nil + }) check := findDoctorCheck(t, c.runDoctor(context.Background()), "zellij") if check.Level != doctorFail || !strings.Contains(check.Message, "require >= 0.44.3") { @@ -44,19 +89,249 @@ func TestDoctorFailsUnsupportedZellijVersion(t *testing.T) { func TestDoctorWarnsWhenZellijMissing(t *testing.T) { setConfigEnv(t) - c := &commandContext{deps: Deps{ + c := doctorContext(t, map[string]string{"git": "/bin/git"}, func(context.Context, string, ...string) ([]byte, error) { + return []byte("git version 2.43.0\n"), nil + }) + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "zellij") + if check.Level != doctorWarn { + t.Fatalf("zellij check = %+v, want WARN", check) + } +} + +func TestDoctorChecksHarnessVersions(t *testing.T) { + setConfigEnv(t) + cmdPath := map[string]string{ + "git": "/bin/git", + "claude": "/bin/claude", + "codex": "/bin/codex", + "aider": "/bin/aider", + "opencode": "/bin/opencode", + } + c := doctorContext(t, cmdPath, func(_ context.Context, name string, args ...string) ([]byte, error) { + switch name { + case "/bin/git": + return []byte("git version 2.43.0\n"), nil + case "/bin/claude", "/bin/codex", "/bin/aider": + if len(args) != 1 || args[0] != "--version" { + t.Fatalf("unexpected harness command: %s %v", name, args) + } + return []byte(strings.TrimPrefix(name, "/bin/") + " 1.2.3\n"), nil + case "/bin/opencode": + if len(args) != 1 || args[0] != "version" { + t.Fatalf("unexpected opencode command: %s %v", name, args) + } + return []byte("opencode 0.9.0\n"), nil + default: + t.Fatalf("unexpected command: %s %v", name, args) + return nil, nil + } + }) + + checks := c.runDoctor(context.Background()) + for _, name := range []string{"claude-code", "codex", "aider", "opencode"} { + check := findDoctorCheck(t, checks, name) + if check.Level != doctorPass || !strings.Contains(check.Message, "resolves to") { + t.Fatalf("%s check = %+v, want PASS with path/version", name, check) + } + } +} + +func TestDoctorWarnsWhenHarnessMissing(t *testing.T) { + setConfigEnv(t) + c := doctorContext(t, map[string]string{"git": "/bin/git"}, func(context.Context, string, ...string) ([]byte, error) { + return []byte("git version 2.43.0\n"), nil + }) + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "codex") + if check.Level != doctorWarn || !strings.Contains(check.Message, "not found in PATH") { + t.Fatalf("codex check = %+v, want WARN missing binary", check) + } +} + +func TestDoctorWarnsWhenHarnessVersionFails(t *testing.T) { + setConfigEnv(t) + c := doctorContext(t, map[string]string{"git": "/bin/git", "codex": "/bin/codex"}, func(_ context.Context, name string, _ ...string) ([]byte, error) { + if name == "/bin/git" { + return []byte("git version 2.43.0\n"), nil + } + return nil, errors.New("boom") + }) + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "codex") + if check.Level != doctorWarn || !strings.Contains(check.Message, "failed") { + t.Fatalf("codex check = %+v, want WARN version failure", check) + } +} + +func TestDoctorChecksGitHubTokenFromEnv(t *testing.T) { + setConfigEnv(t) + srv := githubDoctorServer(t, http.StatusOK, `{"login":"octocat"}`, "repo, read:org") + defer overrideGitHubDoctorBase(t, srv.URL)() + c := doctorContext(t, map[string]string{"git": "/bin/git"}, func(context.Context, string, ...string) ([]byte, error) { + return []byte("git version 2.43.0\n"), nil + }) + t.Setenv("AO_GITHUB_TOKEN", "env-token") + c.deps.HTTPClient = srv.Client() + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "github-token") + if check.Level != doctorPass || !strings.Contains(check.Message, "AO_GITHUB_TOKEN") || !strings.Contains(check.Message, "repo, read:org") { + t.Fatalf("github-token check = %+v, want PASS with source and scopes", check) + } +} + +func TestDoctorChecksGitHubTokenFromGHCLI(t *testing.T) { + setConfigEnv(t) + srv := githubDoctorServer(t, http.StatusOK, `{"login":"octocat"}`, "") + defer overrideGitHubDoctorBase(t, srv.URL)() + c := doctorContext(t, map[string]string{"git": "/bin/git", "gh": "/bin/gh"}, func(_ context.Context, name string, args ...string) ([]byte, error) { + if name == "/bin/gh" { + if len(args) != 2 || args[0] != "auth" || args[1] != "token" { + t.Fatalf("unexpected gh command: %s %v", name, args) + } + return []byte("gh-token\n"), nil + } + return []byte("git version 2.43.0\n"), nil + }) + c.deps.HTTPClient = srv.Client() + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "github-token") + if check.Level != doctorPass || !strings.Contains(check.Message, "gh token valid") { + t.Fatalf("github-token check = %+v, want PASS from gh", check) + } +} + +func TestDoctorWarnsWhenGitHubTokenMissing(t *testing.T) { + setConfigEnv(t) + c := doctorContext(t, map[string]string{"git": "/bin/git"}, func(context.Context, string, ...string) ([]byte, error) { + return []byte("git version 2.43.0\n"), nil + }) + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "github-token") + if check.Level != doctorWarn || !strings.Contains(check.Message, "no GitHub token found") { + t.Fatalf("github-token check = %+v, want WARN missing token", check) + } +} + +func TestDoctorFailsExpiredGitHubToken(t *testing.T) { + setConfigEnv(t) + srv := githubDoctorServer(t, http.StatusUnauthorized, `{"message":"Bad credentials"}`, "") + defer overrideGitHubDoctorBase(t, srv.URL)() + c := doctorContext(t, map[string]string{"git": "/bin/git"}, func(context.Context, string, ...string) ([]byte, error) { + return []byte("git version 2.43.0\n"), nil + }) + t.Setenv("GITHUB_TOKEN", "expired-token") + c.deps.HTTPClient = srv.Client() + + check := findDoctorCheck(t, c.runDoctor(context.Background()), "github-token") + if check.Level != doctorFail || !strings.Contains(check.Message, "HTTP 401") { + t.Fatalf("github-token check = %+v, want FAIL rejected token", check) + } +} + +func TestDoctorJSONOutputIsDecodable(t *testing.T) { + setConfigEnv(t) + clearDoctorGitHubEnv(t) + out, errOut, err := executeCLI(t, Deps{ LookPath: func(name string) (string, error) { if name == "git" { return "/bin/git", nil } return "", errors.New("missing") }, - }.withDefaults()} + CommandOutput: func(context.Context, string, ...string) ([]byte, error) { + return []byte("git version 2.43.0\n"), nil + }, + ProcessAlive: func(int) bool { return false }, + }, "doctor", "--json") + if err != nil { + t.Fatalf("doctor --json failed: %v\nstderr=%s\nstdout=%s", err, errOut, out) + } + var got doctorReport + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("decode doctor json: %v\nout=%s", err, out) + } + if !got.OK || len(got.Checks) == 0 { + t.Fatalf("doctor json = %#v, want ok with checks", got) + } + if findDoctorCheck(t, got.Checks, "git").Section != doctorSectionTools { + t.Fatalf("git json check missing section: %#v", findDoctorCheck(t, got.Checks, "git")) + } +} - check := findDoctorCheck(t, c.runDoctor(context.Background()), "zellij") - if check.Level != doctorWarn { - t.Fatalf("zellij check = %+v, want WARN", check) +func TestDoctorTextOutputIsGrouped(t *testing.T) { + setConfigEnv(t) + clearDoctorGitHubEnv(t) + out, errOut, err := executeCLI(t, Deps{ + LookPath: func(name string) (string, error) { + if name == "git" { + return "/bin/git", nil + } + return "", errors.New("missing") + }, + CommandOutput: func(context.Context, string, ...string) ([]byte, error) { + return []byte("git version 2.43.0\n"), nil + }, + ProcessAlive: func(int) bool { return false }, + }, "doctor") + if err != nil { + t.Fatalf("doctor failed: %v\nstderr=%s\nstdout=%s", err, errOut, out) + } + for _, want := range []string{"Core:\nPASS config:", "Tools:\nPASS git:", "Agent harnesses:\nWARN claude-code:", "WARN codex:", "GitHub:\nWARN github-token:"} { + if !strings.Contains(out, want) { + t.Fatalf("doctor output missing %q:\n%s", want, out) + } + } +} + +func clearDoctorGitHubEnv(t *testing.T) { + t.Helper() + t.Setenv("AO_GITHUB_TOKEN", "") + t.Setenv("GITHUB_TOKEN", "") +} + +func doctorContext(t *testing.T, paths map[string]string, commandOutput func(context.Context, string, ...string) ([]byte, error)) *commandContext { + t.Helper() + clearDoctorGitHubEnv(t) + deps := Deps{ + LookPath: func(name string) (string, error) { + path, ok := paths[name] + if !ok || path == "" { + return "", fmt.Errorf("%s missing", name) + } + return path, nil + }, + ProcessAlive: func(int) bool { return false }, } + if commandOutput != nil { + deps.CommandOutput = commandOutput + } + return &commandContext{deps: deps.withDefaults()} +} + +func githubDoctorServer(t *testing.T, status int, body, scopes string) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != "/user" { + t.Fatalf("unexpected github probe: %s %s", r.Method, r.URL.Path) + } + if got := r.Header.Get("Authorization"); !strings.HasPrefix(got, "Bearer ") { + t.Fatalf("missing bearer auth header: %q", got) + } + if scopes != "" { + w.Header().Set("X-OAuth-Scopes", scopes) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = io.WriteString(w, body) + })) +} + +func overrideGitHubDoctorBase(t *testing.T, base string) func() { + t.Helper() + prev := doctorGitHubRESTBase + doctorGitHubRESTBase = base + return func() { doctorGitHubRESTBase = prev } } func findDoctorCheck(t *testing.T, checks []doctorCheck, name string) doctorCheck { diff --git a/backend/internal/cli/e2e_test.go b/backend/internal/cli/e2e_test.go index f89e46711..e762627bd 100644 --- a/backend/internal/cli/e2e_test.go +++ b/backend/internal/cli/e2e_test.go @@ -80,13 +80,16 @@ func (e env) environ(portOverride string) []string { if strings.HasPrefix(kv, "AO_") { continue } + if strings.HasPrefix(kv, "GITHUB_TOKEN=") || strings.HasPrefix(kv, "GH_TOKEN=") || strings.HasPrefix(kv, "GH_CONFIG_DIR=") { + continue + } out = append(out, kv) } port := fmt.Sprintf("%d", e.port) if portOverride != "" { port = portOverride } - return append(out, "AO_RUN_FILE="+e.runFile, "AO_DATA_DIR="+e.dataDir, "AO_PORT="+port) + return append(out, "AO_RUN_FILE="+e.runFile, "AO_DATA_DIR="+e.dataDir, "AO_PORT="+port, "GH_CONFIG_DIR="+filepath.Join(e.dataDir, "gh-config")) } func freePort(t *testing.T) int { From 5bc4fdd6da37e8e11873ae3cf373ed31bac4d1b4 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Wed, 3 Jun 2026 16:46:59 +0530 Subject: [PATCH 2/2] fix(cli): address doctor review feedback --- backend/internal/cli/doctor.go | 66 +++++++++++++++++------------ backend/internal/cli/doctor_test.go | 31 ++++---------- backend/internal/cli/root.go | 33 +++++++++------ 3 files changed, 68 insertions(+), 62 deletions(-) diff --git a/backend/internal/cli/doctor.go b/backend/internal/cli/doctor.go index 9d257a7a7..5a87a5599 100644 --- a/backend/internal/cli/doctor.go +++ b/backend/internal/cli/doctor.go @@ -5,11 +5,13 @@ import ( "encoding/json" "errors" "fmt" + "io" "io/fs" "net/http" "os" "path/filepath" "regexp" + "strconv" "strings" "github.com/spf13/cobra" @@ -40,16 +42,15 @@ type doctorReport struct { } const ( - doctorSectionCore = "Core" - doctorSectionTools = "Tools" - doctorSectionAgents = "Agent harnesses" - doctorSectionGitHub = "GitHub" - minGitVersion = "2.25.0" - githubDoctorUserAgent = "ao-agent-orchestrator/doctor" + doctorSectionCore = "Core" + doctorSectionTools = "Tools" + doctorSectionAgents = "Agent harnesses" + doctorSectionGitHub = "GitHub" + minGitVersion = "2.25.0" + githubDoctorUserAgent = "ao-agent-orchestrator/doctor" + defaultDoctorGitHubRESTBase = "https://api.github.com" ) -var doctorGitHubRESTBase = "https://api.github.com" - type harnessProbe struct { Name string BinaryName string @@ -59,8 +60,6 @@ type harnessProbe struct { var doctorHarnesses = []harnessProbe{ {Name: "claude-code", BinaryName: "claude", VersionArg: "--version"}, {Name: "codex", BinaryName: "codex", VersionArg: "--version"}, - {Name: "aider", BinaryName: "aider", VersionArg: "--version"}, - {Name: "opencode", BinaryName: "opencode", VersionArg: "version"}, } func newDoctorCommand(ctx *commandContext) *cobra.Command { @@ -237,7 +236,11 @@ func (c *commandContext) checkGit(ctx context.Context) doctorCheck { if err != nil { return doctorCheck{Level: doctorWarn, Section: doctorSectionTools, Name: "git", Message: fmt.Sprintf("%s (version unknown: %s)", path, firstOutputLine(out))} } - if compareDottedVersion(version, minGitVersion) < 0 { + cmp, err := compareDottedVersion(version, minGitVersion) + if err != nil { + return doctorCheck{Level: doctorWarn, Section: doctorSectionTools, Name: "git", Message: fmt.Sprintf("%s (version unknown: %s)", path, firstOutputLine(out))} + } + if cmp < 0 { return doctorCheck{Level: doctorWarn, Section: doctorSectionTools, Name: "git", Message: fmt.Sprintf("%s (version %s; AO expects >= %s for worktrees)", path, version, minGitVersion)} } return doctorCheck{Level: doctorPass, Section: doctorSectionTools, Name: "git", Message: fmt.Sprintf("%s (version %s; supports worktrees)", path, version)} @@ -296,7 +299,7 @@ func (c *commandContext) checkGitHubToken(ctx context.Context) doctorCheck { reqCtx, cancel := context.WithTimeout(ctx, probeTimeout) defer cancel() - req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, strings.TrimRight(doctorGitHubRESTBase, "/")+"/user", http.NoBody) + req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, strings.TrimRight(c.deps.DoctorGitHubRESTBase, "/")+"/user", http.NoBody) if err != nil { return doctorCheck{Level: doctorFail, Section: doctorSectionGitHub, Name: "github-token", Message: err.Error()} } @@ -308,7 +311,10 @@ func (c *commandContext) checkGitHubToken(ctx context.Context) doctorCheck { if err != nil { return doctorCheck{Level: doctorFail, Section: doctorSectionGitHub, Name: "github-token", Message: fmt.Sprintf("%s token validation failed: %v", source, err)} } - defer func() { _ = resp.Body.Close() }() + defer func() { + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + }() if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { return doctorCheck{Level: doctorFail, Section: doctorSectionGitHub, Name: "github-token", Message: fmt.Sprintf("%s token rejected by GitHub (HTTP %d)", source, resp.StatusCode)} @@ -381,9 +387,15 @@ func firstOutputLine(out []byte) string { return strings.TrimSpace(line) } -func compareDottedVersion(a, b string) int { - ap := dottedVersionParts(a) - bp := dottedVersionParts(b) +func compareDottedVersion(a, b string) (int, error) { + ap, err := dottedVersionParts(a) + if err != nil { + return 0, err + } + bp, err := dottedVersionParts(b) + if err != nil { + return 0, err + } maxLen := len(ap) if len(bp) > maxLen { maxLen = len(bp) @@ -398,26 +410,26 @@ func compareDottedVersion(a, b string) int { } switch { case av < bv: - return -1 + return -1, nil case av > bv: - return 1 + return 1, nil } } - return 0 + return 0, nil } -func dottedVersionParts(s string) []int { +func dottedVersionParts(s string) ([]int, error) { raw := strings.Split(s, ".") parts := make([]int, 0, len(raw)) for _, part := range raw { - n := 0 - for _, ch := range part { - if ch < '0' || ch > '9' { - break - } - n = n*10 + int(ch-'0') + if part == "" { + return nil, fmt.Errorf("empty version segment in %q", s) + } + n, err := strconv.Atoi(part) + if err != nil { + return nil, fmt.Errorf("parse version segment %q in %q: %w", part, s, err) } parts = append(parts, n) } - return parts + return parts, nil } diff --git a/backend/internal/cli/doctor_test.go b/backend/internal/cli/doctor_test.go index d495ff715..34e013f11 100644 --- a/backend/internal/cli/doctor_test.go +++ b/backend/internal/cli/doctor_test.go @@ -102,26 +102,19 @@ func TestDoctorWarnsWhenZellijMissing(t *testing.T) { func TestDoctorChecksHarnessVersions(t *testing.T) { setConfigEnv(t) cmdPath := map[string]string{ - "git": "/bin/git", - "claude": "/bin/claude", - "codex": "/bin/codex", - "aider": "/bin/aider", - "opencode": "/bin/opencode", + "git": "/bin/git", + "claude": "/bin/claude", + "codex": "/bin/codex", } c := doctorContext(t, cmdPath, func(_ context.Context, name string, args ...string) ([]byte, error) { switch name { case "/bin/git": return []byte("git version 2.43.0\n"), nil - case "/bin/claude", "/bin/codex", "/bin/aider": + case "/bin/claude", "/bin/codex": if len(args) != 1 || args[0] != "--version" { t.Fatalf("unexpected harness command: %s %v", name, args) } return []byte(strings.TrimPrefix(name, "/bin/") + " 1.2.3\n"), nil - case "/bin/opencode": - if len(args) != 1 || args[0] != "version" { - t.Fatalf("unexpected opencode command: %s %v", name, args) - } - return []byte("opencode 0.9.0\n"), nil default: t.Fatalf("unexpected command: %s %v", name, args) return nil, nil @@ -129,7 +122,7 @@ func TestDoctorChecksHarnessVersions(t *testing.T) { }) checks := c.runDoctor(context.Background()) - for _, name := range []string{"claude-code", "codex", "aider", "opencode"} { + for _, name := range []string{"claude-code", "codex"} { check := findDoctorCheck(t, checks, name) if check.Level != doctorPass || !strings.Contains(check.Message, "resolves to") { t.Fatalf("%s check = %+v, want PASS with path/version", name, check) @@ -167,12 +160,12 @@ func TestDoctorWarnsWhenHarnessVersionFails(t *testing.T) { func TestDoctorChecksGitHubTokenFromEnv(t *testing.T) { setConfigEnv(t) srv := githubDoctorServer(t, http.StatusOK, `{"login":"octocat"}`, "repo, read:org") - defer overrideGitHubDoctorBase(t, srv.URL)() c := doctorContext(t, map[string]string{"git": "/bin/git"}, func(context.Context, string, ...string) ([]byte, error) { return []byte("git version 2.43.0\n"), nil }) t.Setenv("AO_GITHUB_TOKEN", "env-token") c.deps.HTTPClient = srv.Client() + c.deps.DoctorGitHubRESTBase = srv.URL check := findDoctorCheck(t, c.runDoctor(context.Background()), "github-token") if check.Level != doctorPass || !strings.Contains(check.Message, "AO_GITHUB_TOKEN") || !strings.Contains(check.Message, "repo, read:org") { @@ -183,7 +176,6 @@ func TestDoctorChecksGitHubTokenFromEnv(t *testing.T) { func TestDoctorChecksGitHubTokenFromGHCLI(t *testing.T) { setConfigEnv(t) srv := githubDoctorServer(t, http.StatusOK, `{"login":"octocat"}`, "") - defer overrideGitHubDoctorBase(t, srv.URL)() c := doctorContext(t, map[string]string{"git": "/bin/git", "gh": "/bin/gh"}, func(_ context.Context, name string, args ...string) ([]byte, error) { if name == "/bin/gh" { if len(args) != 2 || args[0] != "auth" || args[1] != "token" { @@ -194,6 +186,7 @@ func TestDoctorChecksGitHubTokenFromGHCLI(t *testing.T) { return []byte("git version 2.43.0\n"), nil }) c.deps.HTTPClient = srv.Client() + c.deps.DoctorGitHubRESTBase = srv.URL check := findDoctorCheck(t, c.runDoctor(context.Background()), "github-token") if check.Level != doctorPass || !strings.Contains(check.Message, "gh token valid") { @@ -216,12 +209,12 @@ func TestDoctorWarnsWhenGitHubTokenMissing(t *testing.T) { func TestDoctorFailsExpiredGitHubToken(t *testing.T) { setConfigEnv(t) srv := githubDoctorServer(t, http.StatusUnauthorized, `{"message":"Bad credentials"}`, "") - defer overrideGitHubDoctorBase(t, srv.URL)() c := doctorContext(t, map[string]string{"git": "/bin/git"}, func(context.Context, string, ...string) ([]byte, error) { return []byte("git version 2.43.0\n"), nil }) t.Setenv("GITHUB_TOKEN", "expired-token") c.deps.HTTPClient = srv.Client() + c.deps.DoctorGitHubRESTBase = srv.URL check := findDoctorCheck(t, c.runDoctor(context.Background()), "github-token") if check.Level != doctorFail || !strings.Contains(check.Message, "HTTP 401") { @@ -288,6 +281,7 @@ func clearDoctorGitHubEnv(t *testing.T) { t.Helper() t.Setenv("AO_GITHUB_TOKEN", "") t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") } func doctorContext(t *testing.T, paths map[string]string, commandOutput func(context.Context, string, ...string) ([]byte, error)) *commandContext { @@ -327,13 +321,6 @@ func githubDoctorServer(t *testing.T, status int, body, scopes string) *httptest })) } -func overrideGitHubDoctorBase(t *testing.T, base string) func() { - t.Helper() - prev := doctorGitHubRESTBase - doctorGitHubRESTBase = base - return func() { doctorGitHubRESTBase = prev } -} - func findDoctorCheck(t *testing.T, checks []doctorCheck, name string) doctorCheck { t.Helper() for _, check := range checks { diff --git a/backend/internal/cli/root.go b/backend/internal/cli/root.go index 61e18a0a8..7d5c0df2e 100644 --- a/backend/internal/cli/root.go +++ b/backend/internal/cli/root.go @@ -56,24 +56,28 @@ type Deps struct { ProcessAlive func(pid int) bool LookPath func(file string) (string, error) CommandOutput func(ctx context.Context, name string, args ...string) ([]byte, error) - Now func() time.Time - Sleep func(time.Duration) + // DoctorGitHubRESTBase lets tests point the doctor GitHub token probe at + // httptest without mutating package-global state. + DoctorGitHubRESTBase string + Now func() time.Time + Sleep func(time.Duration) } // DefaultDeps returns production dependencies. func DefaultDeps() Deps { return Deps{ - In: os.Stdin, - Out: os.Stdout, - Err: os.Stderr, - HTTPClient: &http.Client{Timeout: 2 * time.Second}, - Executable: os.Executable, - StartProcess: startProcess, - ProcessAlive: processalive.Alive, - LookPath: exec.LookPath, - CommandOutput: commandOutput, - Now: time.Now, - Sleep: time.Sleep, + In: os.Stdin, + Out: os.Stdout, + Err: os.Stderr, + HTTPClient: &http.Client{Timeout: 2 * time.Second}, + Executable: os.Executable, + StartProcess: startProcess, + ProcessAlive: processalive.Alive, + LookPath: exec.LookPath, + CommandOutput: commandOutput, + DoctorGitHubRESTBase: defaultDoctorGitHubRESTBase, + Now: time.Now, + Sleep: time.Sleep, } } @@ -110,6 +114,9 @@ func (d Deps) withDefaults() Deps { if d.CommandOutput == nil { d.CommandOutput = def.CommandOutput } + if d.DoctorGitHubRESTBase == "" { + d.DoctorGitHubRESTBase = def.DoctorGitHubRESTBase + } if d.Now == nil { d.Now = def.Now }