-
Notifications
You must be signed in to change notification settings - Fork 388
feat(cli): health-aware list/stop, startup failure diagnostics, port retry, doctor --probe #820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9d78484
f0b4284
4ad6ba9
279582d
752ac91
2a8d7da
4e1268f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,9 @@ type DoctorReport struct { | |
| ProviderKeys map[string]ProviderKey `json:"provider_keys"` | ||
| ControlPlane ControlPlaneStatus `json:"control_plane"` | ||
| Recommendation Recommendation `json:"recommendation"` | ||
| // HarnessProbes is populated only when `--probe` is passed: a live smoke | ||
| // test of each detected provider CLI, keyed by provider name. | ||
| HarnessProbes map[string]HarnessProbeResult `json:"harness_probes,omitempty"` | ||
| } | ||
|
|
||
| // ToolStatus describes whether a CLI is available and, if so, where. | ||
|
|
@@ -78,19 +81,37 @@ var providerEnvVars = []struct { | |
|
|
||
| // harnessProviders is the canonical list of CLIs `app.harness()` knows how to drive. | ||
| var harnessProviders = []struct { | ||
| Name string // value passed to provider= in app.harness() | ||
| Binary string // executable name to look up on PATH | ||
| Name string // value passed to provider= in app.harness() | ||
| Binary string // executable name to look up on PATH | ||
| ProbeArgs []string // minimal one-shot invocation used by `--probe` | ||
| }{ | ||
| {Name: "claude-code", Binary: "claude"}, | ||
| {Name: "codex", Binary: "codex"}, | ||
| {Name: "gemini", Binary: "gemini"}, | ||
| {Name: "opencode", Binary: "opencode"}, | ||
| {Name: "claude-code", Binary: "claude", ProbeArgs: []string{"-p", "Say OK"}}, | ||
| {Name: "codex", Binary: "codex", ProbeArgs: []string{"exec", "Say OK"}}, | ||
| {Name: "gemini", Binary: "gemini", ProbeArgs: []string{"-p", "Say OK"}}, | ||
| {Name: "opencode", Binary: "opencode", ProbeArgs: []string{"run", "Say OK"}}, | ||
| } | ||
|
|
||
| // harnessProbeTimeout bounds a single provider smoke test. Coding-agent CLIs | ||
| // cold-start slowly (model download, auth handshake), so this is generous. | ||
| const harnessProbeTimeout = 60 * time.Second | ||
|
|
||
| // HarnessProbeResult is the outcome of smoke-testing one provider CLI with a | ||
| // minimal one-shot prompt. | ||
| type HarnessProbeResult struct { | ||
| Provider string `json:"provider"` | ||
| Binary string `json:"binary,omitempty"` | ||
| // Status is one of: ok, empty, error, timeout. | ||
| Status string `json:"status"` | ||
| DurationMS int64 `json:"duration_ms"` | ||
| // Detail carries a short explanation on failure (stderr head / note). | ||
| Detail string `json:"detail,omitempty"` | ||
| } | ||
|
|
||
| // NewDoctorCommand builds the `af doctor` command. | ||
| func NewDoctorCommand() *cobra.Command { | ||
| var jsonOut bool | ||
| var controlPlaneURL string | ||
| var probe bool | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "doctor", | ||
|
|
@@ -108,13 +129,25 @@ Coding agents and skills (e.g. agentfield) should call this once at the | |
| start of a build to learn ground truth instead of probing each tool | ||
| by hand. | ||
|
|
||
| With --probe, each DETECTED provider CLI is additionally smoke-tested with a | ||
| minimal one-shot prompt (e.g. codex exec "Say OK") and classified as ok, empty | ||
| (exit 0 but no completion — a silently broken provider), error, or timeout. | ||
| Detecting a binary on PATH does not prove it can actually complete; --probe | ||
| catches providers that look installed but return nothing. Each probe runs a real | ||
| one-shot request and consumes a trivial amount of provider quota. | ||
|
|
||
| Examples: | ||
| af doctor # Pretty human-readable output | ||
| af doctor --json # Machine-readable JSON for tooling/skills | ||
| af doctor --probe # Also smoke-test each detected provider CLI | ||
| af doctor --json | jq # Pipe to jq for filtering`, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| report := buildDoctorReport(controlPlaneURL) | ||
|
|
||
| if probe { | ||
| report.HarnessProbes = runHarnessProbes(report) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note Advisory — non-blocking. Safe to merge and address in a follow-up. Why non-blocking: Bug in new --probe feature does not break existing contracts or cause security/data issues; can be fixed in a follow-up. Make 🟠 A CLI that returns Evidence
💡 Suggested Fix When
🤖 Reviewed by AgentField PR-AF |
||
| } | ||
|
|
||
| if jsonOut { | ||
| enc := json.NewEncoder(os.Stdout) | ||
| enc.SetIndent("", " ") | ||
|
|
@@ -128,10 +161,104 @@ Examples: | |
|
|
||
| cmd.Flags().BoolVar(&jsonOut, "json", false, "Output the report as JSON (recommended for tools and skills)") | ||
| cmd.Flags().StringVar(&controlPlaneURL, "server", "http://localhost:8080", "Control plane URL to probe for /api/v1/health") | ||
| cmd.Flags().BoolVar(&probe, "probe", false, "Smoke-test each detected provider CLI with a minimal one-shot prompt (consumes trivial provider quota)") | ||
|
|
||
| return cmd | ||
| } | ||
|
|
||
| // runHarnessProbes smoke-tests every provider CLI that doctor detected on PATH. | ||
| // Providers that were not detected are skipped entirely — probing runs only for | ||
| // what doctor already reports as available. | ||
| func runHarnessProbes(report DoctorReport) map[string]HarnessProbeResult { | ||
| results := map[string]HarnessProbeResult{} | ||
| for _, h := range harnessProviders { | ||
| if !report.HarnessProviders[h.Name].Available { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note Advisory — non-blocking. Safe to merge and address in a follow-up. Why non-blocking: The issue is about defensive validation in a diagnostic tool, not a proven production impact. Only mark a harness provider as available for probing after the version command succeeds.
Evidence
🤖 Reviewed by AgentField PR-AF |
||
| continue | ||
| } | ||
| results[h.Name] = probeHarnessProvider(h.Name, h.Binary, h.ProbeArgs, harnessProbeTimeout) | ||
| } | ||
| return results | ||
| } | ||
|
|
||
| // probeHarnessProvider runs one provider CLI's minimal one-shot invocation and | ||
| // classifies the outcome. | ||
| func probeHarnessProvider(name, binary string, args []string, timeout time.Duration) HarnessProbeResult { | ||
| start := time.Now() | ||
| stdout, stderr, exitCode, timedOut := runProbeCommand(binary, args, timeout) | ||
| status := classifyProbe(exitCode, stdout, timedOut) | ||
|
|
||
| result := HarnessProbeResult{ | ||
| Provider: name, | ||
| Binary: binary, | ||
| Status: status, | ||
| DurationMS: time.Since(start).Milliseconds(), | ||
| } | ||
| switch status { | ||
| case "error": | ||
| result.Detail = firstLine(stderr) | ||
| case "timeout": | ||
| result.Detail = fmt.Sprintf("no response within %s", timeout) | ||
| case "empty": | ||
| result.Detail = "exited 0 with no completion output" | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| // runProbeCommand executes bin with args under a timeout, returning stdout, | ||
| // stderr, the process exit code, and whether the timeout fired. A timeout is | ||
| // reported distinctly so it is never misclassified as a plain error. | ||
| func runProbeCommand(bin string, args []string, timeout time.Duration) (stdout, stderr string, exitCode int, timedOut bool) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), timeout) | ||
| defer cancel() | ||
|
|
||
| cmd := exec.CommandContext(ctx, bin, args...) | ||
| var outBuf, errBuf strings.Builder | ||
| cmd.Stdout = &outBuf | ||
| cmd.Stderr = &errBuf | ||
| err := cmd.Run() | ||
|
|
||
| if ctx.Err() == context.DeadlineExceeded { | ||
| return outBuf.String(), errBuf.String(), -1, true | ||
| } | ||
| if err != nil { | ||
| if exitErr, ok := err.(*exec.ExitError); ok { | ||
| return outBuf.String(), errBuf.String(), exitErr.ExitCode(), false | ||
| } | ||
| // Could not start (binary vanished between detection and probe) — treat | ||
| // as a non-zero exit so it classifies as error. | ||
| return outBuf.String(), err.Error(), 1, false | ||
| } | ||
| return outBuf.String(), errBuf.String(), 0, false | ||
| } | ||
|
|
||
| // classifyProbe maps a probe outcome to a status. Order matters: a timeout is | ||
| // checked before the exit code (a killed process also exits non-zero), and an | ||
| // empty completion on a clean exit is the real-world "silently broken provider" | ||
| // case that a mere PATH check misses. | ||
| func classifyProbe(exitCode int, stdout string, timedOut bool) string { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note Advisory — non-blocking. Safe to merge and address in a follow-up. Why non-blocking: Diagnostic tool misreporting does not break build, security, data, or API contracts. Classify OpenCode's exit-zero stderr failures as errors 🟠 Pass stderr into classification and treat OpenCode's exit-zero stderr errors as errors.
Evidence
💡 Suggested Fix Pass stderr into classification and, before returning
🤖 Reviewed by AgentField PR-AF |
||
| switch { | ||
| case timedOut: | ||
| return "timeout" | ||
| case exitCode != 0: | ||
| return "error" | ||
| case strings.TrimSpace(stdout) == "": | ||
| return "empty" | ||
| default: | ||
| return "ok" | ||
| } | ||
| } | ||
|
|
||
| // firstLine returns the first non-empty line of s, trimmed, for compact error | ||
| // detail. | ||
| func firstLine(s string) string { | ||
| for _, line := range strings.Split(s, "\n") { | ||
| if trimmed := strings.TrimSpace(line); trimmed != "" { | ||
| return trimmed | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| // buildDoctorReport collects the full environment snapshot. | ||
| func buildDoctorReport(controlPlaneURL string) DoctorReport { | ||
| report := DoctorReport{ | ||
|
|
@@ -284,6 +411,19 @@ func printDoctorReport(r DoctorReport) { | |
| } | ||
| fmt.Println() | ||
|
|
||
| if len(r.HarnessProbes) > 0 { | ||
| bold.Println("Harness provider probes (--probe)") | ||
| for _, h := range harnessProviders { | ||
| probe, ok := r.HarnessProbes[h.Name] | ||
| if !ok { | ||
| continue | ||
| } | ||
| printProbeLine(probe, green, red, yellow) | ||
| } | ||
| fmt.Println(" note: probes ran a real one-shot request and consumed a trivial amount of provider quota") | ||
| fmt.Println() | ||
| } | ||
|
|
||
| bold.Println("Provider API keys") | ||
| for _, p := range providerEnvVars { | ||
| key := r.ProviderKeys[p.Name] | ||
|
|
@@ -329,6 +469,23 @@ func printDoctorReport(r DoctorReport) { | |
| fmt.Println("Tip: pipe to jq for tooling — `af doctor --json | jq`") | ||
| } | ||
|
|
||
| // printProbeLine renders one provider probe result with a status-colored mark. | ||
| func printProbeLine(p HarnessProbeResult, ok, fail, warn *color.Color) { | ||
| mark, c := "✓", ok | ||
| switch p.Status { | ||
| case "empty", "timeout": | ||
| mark, c = "⚠", warn | ||
| case "error": | ||
| mark, c = "✗", fail | ||
| } | ||
| c.Printf(" %s %-12s %s", mark, p.Provider, p.Status) | ||
| fmt.Printf(" (%dms)", p.DurationMS) | ||
| if p.Detail != "" { | ||
| fmt.Printf(" — %s", p.Detail) | ||
| } | ||
| fmt.Println() | ||
| } | ||
|
|
||
| func printToolLine(name string, status ToolStatus, ok, fail *color.Color) { | ||
| if status.Available { | ||
| ok.Printf(" ✓ %-12s %s", name, status.Path) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "io" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "os" | ||
| "os/exec" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| // Contract: classifyProbe maps (exit code, stdout, timed-out) to the four probe | ||
| // statuses, with timeout taking precedence over a non-zero exit and an empty | ||
| // completion on a clean exit reported distinctly from an error. | ||
| func TestClassifyProbe(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| exitCode int | ||
| stdout string | ||
| timedOut bool | ||
| want string | ||
| }{ | ||
| {"ok", 0, "OK\n", false, "ok"}, | ||
| {"empty exit zero", 0, "", false, "empty"}, | ||
| {"empty whitespace only", 0, " \n\t", false, "empty"}, | ||
| {"error nonzero", 1, "partial", false, "error"}, | ||
| {"error nonzero no output", 127, "", false, "error"}, | ||
| {"timeout wins over exit code", -1, "", true, "timeout"}, | ||
| {"timeout wins even with output", 1, "some", true, "timeout"}, | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| if got := classifyProbe(tc.exitCode, tc.stdout, tc.timedOut); got != tc.want { | ||
| t.Errorf("classifyProbe(%d, %q, %v) = %q, want %q", tc.exitCode, tc.stdout, tc.timedOut, got, tc.want) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // End-to-end wiring of runProbeCommand -> classifyProbe over real processes, so | ||
| // each classification path is exercised through the actual command runner. | ||
| func TestProbeHarnessProvider_RealProcesses(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| bin string | ||
| args []string | ||
| timeout time.Duration | ||
| want string | ||
| }{ | ||
| {"ok", "echo", []string{"OK"}, 5 * time.Second, "ok"}, | ||
| {"empty", "true", nil, 5 * time.Second, "empty"}, | ||
| {"error", "false", nil, 5 * time.Second, "error"}, | ||
| {"timeout", "sleep", []string{"5"}, 200 * time.Millisecond, "timeout"}, | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| if _, err := exec.LookPath(tc.bin); err != nil { | ||
| t.Skipf("%s not available: %v", tc.bin, err) | ||
| } | ||
| res := probeHarnessProvider("prov-"+tc.name, tc.bin, tc.args, tc.timeout) | ||
| if res.Status != tc.want { | ||
| t.Errorf("status = %q, want %q (detail=%q)", res.Status, tc.want, res.Detail) | ||
| } | ||
| if res.Provider != "prov-"+tc.name { | ||
| t.Errorf("provider label lost: %q", res.Provider) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // Contract: probes run ONLY for providers doctor already detected — unavailable | ||
| // providers are never invoked. | ||
| func TestRunHarnessProbes_SkipsUndetected(t *testing.T) { | ||
| report := DoctorReport{ | ||
| HarnessProviders: map[string]ToolStatus{ | ||
| "claude-code": {Available: false}, | ||
| "codex": {Available: false}, | ||
| "gemini": {Available: false}, | ||
| "opencode": {Available: false}, | ||
| }, | ||
| } | ||
| got := runHarnessProbes(report) | ||
| if len(got) != 0 { | ||
| t.Errorf("no detected providers should mean no probes, got %v", got) | ||
| } | ||
| } | ||
|
|
||
| // Contract: `af doctor` without --probe performs no probe (no harness_probes in | ||
| // the report). | ||
| func TestDoctorCommand_NoProbeByDefault(t *testing.T) { | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| out := captureStdoutCLI(t, func() { | ||
| cmd := NewDoctorCommand() | ||
| cmd.SetArgs([]string{"--json", "--server", srv.URL}) | ||
| if err := cmd.Execute(); err != nil { | ||
| t.Fatalf("doctor failed: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| var report DoctorReport | ||
| if err := json.Unmarshal([]byte(out), &report); err != nil { | ||
| t.Fatalf("parse report: %v\noutput:\n%s", err, out) | ||
| } | ||
| if len(report.HarnessProbes) != 0 { | ||
| t.Errorf("without --probe there must be no probes, got %v", report.HarnessProbes) | ||
| } | ||
| } | ||
|
|
||
| // captureStdoutCLI captures os.Stdout while fn runs. | ||
| func captureStdoutCLI(t *testing.T, fn func()) string { | ||
| t.Helper() | ||
| orig := os.Stdout | ||
| r, w, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatalf("pipe: %v", err) | ||
| } | ||
| os.Stdout = w | ||
| done := make(chan string, 1) | ||
| go func() { | ||
| var b bytes.Buffer | ||
| _, _ = io.Copy(&b, r) | ||
| done <- b.String() | ||
| }() | ||
| fn() | ||
| _ = w.Close() | ||
| os.Stdout = orig | ||
| return <-done | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: This is a false negative in a diagnostic tool, not a production-breaking issue.
🟠
IMPORTANT· confidence 98%Remove the PATH-based
claudebinary check fromclaude-codeharness health inaf doctor.af doctortreatsclaude-codeas usable only when a globalclaudeexecutable is on PATH, but the actual Pythonapp.harness(provider="claude-code")implementation uses theclaude_agent_sdkpackage and explicitly does not require that binary. Thus a valid Claude Code harness installation can be reported unavailable (and cannot be probed) solely becauseclaudeis absent from PATH.Evidence
🤖 Reviewed by AgentField PR-AF