From 441dffc1f48d2f9fb5ac28ee8532b0ed29399267 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Fri, 17 Apr 2026 17:08:50 -0400 Subject: [PATCH 1/3] refactor(runners): consolidate probe core with Status enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the /v1/models probe out of package cmd into runners so LocalRunner and the probe-local CLI share a single reachability path. Addresses the tri-review findings: - new runners.Probe replaces cmd.runProbe; ProbeConfig/ProbeResult now live on the runners package where sharing them with LocalRunner is legitimate (unexports them from cmd) - ProbeStatus enum (disabled/unreachable/endpoint_error/invalid_response/ model_missing/healthy) replaces the Reachable+HTTPStatus+ModelMatch booleans, removing the 2xx-with-bad-JSON ambiguity - ModelsSeen initialized to []string{} so JSON output emits [] not null - LocalRunner.Available now delegates to Probe and accepts healthy or model_missing (preserves current permissive semantics — model checks stay the caller's concern) - unreachable hint distinguishes context.DeadlineExceeded from context.Canceled from generic network failure; covered by a new TestProbe_CallerCanceled cmd/probe_local.go now holds only the Cobra command and the two formatters; all probe mechanics are in runners/probe.go. --- src/cmd/probe_local.go | 158 ++++------------------- src/cmd/probe_local_test.go | 246 +++++++----------------------------- src/runners/local.go | 32 ++--- src/runners/local_test.go | 5 +- src/runners/probe.go | 151 ++++++++++++++++++++++ src/runners/probe_test.go | 211 +++++++++++++++++++++++++++++++ 6 files changed, 449 insertions(+), 354 deletions(-) create mode 100644 src/runners/probe.go create mode 100644 src/runners/probe_test.go diff --git a/src/cmd/probe_local.go b/src/cmd/probe_local.go index a631a08..80ea408 100644 --- a/src/cmd/probe_local.go +++ b/src/cmd/probe_local.go @@ -1,12 +1,9 @@ package cmd import ( - "context" "encoding/json" "errors" "fmt" - "io" - "net/http" "strings" "time" @@ -16,107 +13,7 @@ import ( var errProbeFailed = errors.New("probe failed") -type ProbeConfig struct { - Enabled bool - Endpoint string - Model string - APIKey string - Timeout time.Duration -} - -type ProbeResult struct { - Endpoint string `json:"endpoint"` - Model string `json:"model"` - Enabled bool `json:"enabled"` - Reachable bool `json:"reachable"` - HTTPStatus int `json:"http_status"` - LatencyMS int64 `json:"latency_ms"` - ModelsSeen []string `json:"models_seen"` - ModelMatch bool `json:"model_match"` - ErrorMsg string `json:"error,omitempty"` - Hint string `json:"hint,omitempty"` -} - -func runProbe(ctx context.Context, cfg ProbeConfig) ProbeResult { - r := ProbeResult{ - Endpoint: cfg.Endpoint, - Model: cfg.Model, - Enabled: cfg.Enabled, - } - if !cfg.Enabled { - return r - } - - reqCtx, cancel := context.WithTimeout(ctx, cfg.Timeout) - defer cancel() - - req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, cfg.Endpoint+"/models", nil) - if err != nil { - r.ErrorMsg = fmt.Sprintf("building request: %v", err) - r.Hint = "check DEVKIT_LOCAL_ENDPOINT format — must be a valid URL ending in /v1" - return r - } - if cfg.APIKey != "" { - req.Header.Set("Authorization", "Bearer "+cfg.APIKey) - } - - client := &http.Client{Timeout: cfg.Timeout} - start := time.Now() - resp, err := client.Do(req) - r.LatencyMS = time.Since(start).Milliseconds() - if err != nil { - r.ErrorMsg = err.Error() - r.Hint = "endpoint unreachable — check it is running and DEVKIT_LOCAL_ENDPOINT is correct" - return r - } - defer resp.Body.Close() - r.HTTPStatus = resp.StatusCode - - if resp.StatusCode >= 400 { - body, _ := io.ReadAll(io.LimitReader(resp.Body, 256)) - r.ErrorMsg = strings.TrimSpace(string(body)) - switch resp.StatusCode { - case 401, 403: - r.Hint = "check DEVKIT_LOCAL_API_KEY — endpoint requires auth" - case 404: - r.Hint = "check DEVKIT_LOCAL_ENDPOINT — must end in /v1 (some stacks need the suffix)" - default: - r.Hint = "endpoint returned an error — check server logs" - } - return r - } - - raw, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) - if err != nil { - r.ErrorMsg = fmt.Sprintf("reading response: %v", err) - return r - } - - var parsed struct { - Data []struct { - ID string `json:"id"` - } `json:"data"` - } - if err := json.Unmarshal(raw, &parsed); err != nil { - r.ErrorMsg = fmt.Sprintf("parsing /models response: %v", err) - r.Hint = "endpoint did not return OpenAI /models JSON — confirm it speaks the OpenAI spec" - return r - } - - r.Reachable = true - for _, m := range parsed.Data { - r.ModelsSeen = append(r.ModelsSeen, m.ID) - if m.ID == cfg.Model { - r.ModelMatch = true - } - } - if !r.ModelMatch { - r.Hint = fmt.Sprintf("configured model %q not in /models — check DEVKIT_LOCAL_MODEL matches a name the server exposes", cfg.Model) - } - return r -} - -func formatHuman(r ProbeResult) string { +func formatHuman(r runners.ProbeResult) string { var b strings.Builder fmt.Fprintf(&b, "endpoint: %s\n", r.Endpoint) fmt.Fprintf(&b, "model: %s\n", r.Model) @@ -125,38 +22,40 @@ func formatHuman(r ProbeResult) string { return b.String() } fmt.Fprintln(&b, "enabled: yes") - if r.Reachable { + + switch r.Status { + case runners.ProbeHealthy: + fmt.Fprintf(&b, "reachable: yes (HTTP %d, %dms)\n", r.HTTPStatus, r.LatencyMS) + fmt.Fprintf(&b, "models seen: %s\n", strings.Join(r.ModelsSeen, ", ")) + fmt.Fprintln(&b, "model match: OK (configured model present in /models)") + case runners.ProbeModelMissing: fmt.Fprintf(&b, "reachable: yes (HTTP %d, %dms)\n", r.HTTPStatus, r.LatencyMS) if len(r.ModelsSeen) > 0 { fmt.Fprintf(&b, "models seen: %s\n", strings.Join(r.ModelsSeen, ", ")) } else { fmt.Fprintln(&b, "models seen: (none returned)") } - if r.ModelMatch { - fmt.Fprintln(&b, "model match: OK (configured model present in /models)") + fmt.Fprintln(&b, "model match: MISSING") + if r.Hint != "" { + fmt.Fprintf(&b, "hint: %s\n", r.Hint) + } + default: + if r.HTTPStatus > 0 { + fmt.Fprintf(&b, "reachable: NO (HTTP %d in %dms)\n", r.HTTPStatus, r.LatencyMS) } else { - fmt.Fprintln(&b, "model match: MISSING") - if r.Hint != "" { - fmt.Fprintf(&b, "hint: %s\n", r.Hint) - } + fmt.Fprintf(&b, "reachable: NO (%dms)\n", r.LatencyMS) + } + if r.Hint != "" { + fmt.Fprintf(&b, "hint: %s\n", r.Hint) + } + if r.ErrorMsg != "" { + fmt.Fprintf(&b, "body: %s\n", r.ErrorMsg) } - return b.String() - } - if r.HTTPStatus > 0 { - fmt.Fprintf(&b, "reachable: NO (HTTP %d in %dms)\n", r.HTTPStatus, r.LatencyMS) - } else { - fmt.Fprintf(&b, "reachable: NO (%dms)\n", r.LatencyMS) - } - if r.Hint != "" { - fmt.Fprintf(&b, "hint: %s\n", r.Hint) - } - if r.ErrorMsg != "" { - fmt.Fprintf(&b, "body: %s\n", r.ErrorMsg) } return b.String() } -func formatJSON(r ProbeResult) ([]byte, error) { +func formatJSON(r runners.ProbeResult) ([]byte, error) { return json.MarshalIndent(r, "", " ") } @@ -174,14 +73,14 @@ present in the server's /v1/models response. Exit 0 on healthy, 1 otherwise.`, SilenceUsage: true, SilenceErrors: true, RunE: func(cmd *cobra.Command, args []string) error { - cfg := ProbeConfig{ + cfg := runners.ProbeConfig{ Enabled: runners.LocalEnabled(), Endpoint: runners.LocalEndpoint(), Model: runners.LocalModel(), APIKey: runners.LocalAPIKey(), Timeout: 3 * time.Second, } - result := runProbe(cmd.Context(), cfg) + result := runners.Probe(cmd.Context(), cfg) if probeLocalJSON { out, err := formatJSON(result) @@ -193,13 +92,10 @@ present in the server's /v1/models response. Exit 0 on healthy, 1 otherwise.`, fmt.Print(formatHuman(result)) } - if !cfg.Enabled { + if result.Status == runners.ProbeDisabled || result.Status == runners.ProbeHealthy { return nil } - if !result.Reachable || !result.ModelMatch { - return errProbeFailed - } - return nil + return errProbeFailed }, } diff --git a/src/cmd/probe_local_test.go b/src/cmd/probe_local_test.go index b7878d5..fc691e6 100644 --- a/src/cmd/probe_local_test.go +++ b/src/cmd/probe_local_test.go @@ -1,211 +1,22 @@ package cmd import ( - "context" "encoding/json" - "fmt" - "net/http" - "net/http/httptest" "strings" "testing" - "time" -) - -func TestRunProbe_Disabled(t *testing.T) { - cfg := ProbeConfig{ - Enabled: false, - Endpoint: "http://localhost:8080/v1", - Model: "test-model", - Timeout: 1 * time.Second, - } - got := runProbe(context.Background(), cfg) - - if got.Enabled { - t.Errorf("Enabled: got true, want false") - } - if got.Reachable { - t.Errorf("Reachable: got true, want false (disabled should not probe)") - } - if got.HTTPStatus != 0 { - t.Errorf("HTTPStatus: got %d, want 0 (no request should fire)", got.HTTPStatus) - } -} - -func TestRunProbe_HappyPath(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/v1/models" { - http.NotFound(w, r) - return - } - w.Header().Set("Content-Type", "application/json") - fmt.Fprint(w, `{"data":[{"id":"gemma-4-26b-a4b"},{"id":"mistral-small-4"}]}`) - })) - defer srv.Close() - - cfg := ProbeConfig{ - Enabled: true, - Endpoint: srv.URL + "/v1", - Model: "gemma-4-26b-a4b", - Timeout: 2 * time.Second, - } - got := runProbe(context.Background(), cfg) - - if !got.Reachable { - t.Fatalf("Reachable: got false, want true (err=%q)", got.ErrorMsg) - } - if got.HTTPStatus != 200 { - t.Errorf("HTTPStatus: got %d, want 200", got.HTTPStatus) - } - if !got.ModelMatch { - t.Errorf("ModelMatch: got false, want true (models=%v)", got.ModelsSeen) - } - wantModels := []string{"gemma-4-26b-a4b", "mistral-small-4"} - if len(got.ModelsSeen) != len(wantModels) { - t.Fatalf("ModelsSeen len: got %d, want %d", len(got.ModelsSeen), len(wantModels)) - } - for i, m := range wantModels { - if got.ModelsSeen[i] != m { - t.Errorf("ModelsSeen[%d]: got %q, want %q", i, got.ModelsSeen[i], m) - } - } -} - -func TestRunProbe_ModelMissing(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - fmt.Fprint(w, `{"data":[{"id":"something-else"}]}`) - })) - defer srv.Close() - - got := runProbe(context.Background(), ProbeConfig{ - Enabled: true, Endpoint: srv.URL + "/v1", Model: "not-there", Timeout: 2 * time.Second, - }) - - if !got.Reachable { - t.Fatalf("Reachable: got false, want true") - } - if got.ModelMatch { - t.Errorf("ModelMatch: got true, want false") - } - if got.Hint == "" { - t.Errorf("Hint: got empty, want actionable text about DEVKIT_LOCAL_MODEL") - } -} - -func TestRunProbe_Unauthorized(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusUnauthorized) - fmt.Fprint(w, `{"error":"unauthorized"}`) - })) - defer srv.Close() - - got := runProbe(context.Background(), ProbeConfig{ - Enabled: true, Endpoint: srv.URL + "/v1", Model: "m", Timeout: 2 * time.Second, - }) - - if got.Reachable { - t.Errorf("Reachable: got true, want false on 401") - } - if got.HTTPStatus != 401 { - t.Errorf("HTTPStatus: got %d, want 401", got.HTTPStatus) - } - if !strings.Contains(got.Hint, "DEVKIT_LOCAL_API_KEY") { - t.Errorf("Hint: got %q, want mention of DEVKIT_LOCAL_API_KEY", got.Hint) - } -} - -func TestRunProbe_NotFound(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - http.NotFound(w, r) - })) - defer srv.Close() - - got := runProbe(context.Background(), ProbeConfig{ - Enabled: true, Endpoint: srv.URL + "/wrong", Model: "m", Timeout: 2 * time.Second, - }) - - if got.Reachable { - t.Errorf("Reachable: got true, want false on 404") - } - if got.HTTPStatus != 404 { - t.Errorf("HTTPStatus: got %d, want 404", got.HTTPStatus) - } - if !strings.Contains(got.Hint, "/v1") { - t.Errorf("Hint: got %q, want mention of /v1 suffix", got.Hint) - } -} - -func TestRunProbe_ServerError(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprint(w, `{"error":"boom"}`) - })) - defer srv.Close() - - got := runProbe(context.Background(), ProbeConfig{ - Enabled: true, Endpoint: srv.URL + "/v1", Model: "m", Timeout: 2 * time.Second, - }) - - if got.Reachable { - t.Errorf("Reachable: got true, want false on 500") - } - if got.HTTPStatus != 500 { - t.Errorf("HTTPStatus: got %d, want 500", got.HTTPStatus) - } - if !strings.Contains(got.Hint, "server logs") { - t.Errorf("Hint: got %q, want mention of server logs (default 5xx branch)", got.Hint) - } - if got.ErrorMsg == "" { - t.Errorf("ErrorMsg: got empty, want response body snippet") - } -} - -func TestRunProbe_Timeout(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(300 * time.Millisecond) - fmt.Fprint(w, `{"data":[]}`) - })) - defer srv.Close() - got := runProbe(context.Background(), ProbeConfig{ - Enabled: true, Endpoint: srv.URL + "/v1", Model: "m", Timeout: 50 * time.Millisecond, - }) - - if got.Reachable { - t.Errorf("Reachable: got true, want false on timeout") - } - if got.ErrorMsg == "" { - t.Errorf("ErrorMsg: got empty, want timeout error text") - } -} - -func TestRunProbe_BadJSON(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - fmt.Fprint(w, `not json at all`) - })) - defer srv.Close() - - got := runProbe(context.Background(), ProbeConfig{ - Enabled: true, Endpoint: srv.URL + "/v1", Model: "m", Timeout: 2 * time.Second, - }) - - if got.Reachable { - t.Errorf("Reachable: got true, want false on invalid JSON") - } - if !strings.Contains(got.ErrorMsg, "parsing") { - t.Errorf("ErrorMsg: got %q, want mention of parse error", got.ErrorMsg) - } -} + "github.com/5uck1ess/devkit/runners" +) func TestFormatHuman_HappyPath(t *testing.T) { - r := ProbeResult{ + r := runners.ProbeResult{ Endpoint: "http://host:8080/v1", Model: "gemma", Enabled: true, - Reachable: true, + Status: runners.ProbeHealthy, HTTPStatus: 200, LatencyMS: 123, ModelsSeen: []string{"gemma", "mistral"}, - ModelMatch: true, } out := formatHuman(r) @@ -224,17 +35,18 @@ func TestFormatHuman_HappyPath(t *testing.T) { } func TestFormatHuman_Disabled(t *testing.T) { - out := formatHuman(ProbeResult{Endpoint: "http://x/v1", Model: "y", Enabled: false}) + out := formatHuman(runners.ProbeResult{Endpoint: "http://x/v1", Model: "y", Enabled: false}) if !strings.Contains(out, "disabled") { t.Errorf("disabled output missing 'disabled' marker. Got:\n%s", out) } } func TestFormatHuman_Unreachable(t *testing.T) { - r := ProbeResult{ + r := runners.ProbeResult{ Endpoint: "http://x/v1", Model: "y", Enabled: true, - HTTPStatus: 401, ErrorMsg: `{"error":"nope"}`, - Hint: "check DEVKIT_LOCAL_API_KEY — endpoint requires auth", + Status: runners.ProbeEndpointError, HTTPStatus: 401, + ErrorMsg: `{"error":"nope"}`, + Hint: "check DEVKIT_LOCAL_API_KEY — endpoint requires auth", } out := formatHuman(r) @@ -245,11 +57,27 @@ func TestFormatHuman_Unreachable(t *testing.T) { } } +func TestFormatHuman_ModelMissing(t *testing.T) { + r := runners.ProbeResult{ + Endpoint: "http://x/v1", Model: "y", Enabled: true, + Status: runners.ProbeModelMissing, HTTPStatus: 200, LatencyMS: 42, + ModelsSeen: []string{"z"}, + Hint: "configured model \"y\" not in /models", + } + out := formatHuman(r) + + for _, want := range []string{"reachable: yes", "HTTP 200", "models seen: z", "model match: MISSING", "hint:"} { + if !strings.Contains(out, want) { + t.Errorf("output missing %q. Got:\n%s", want, out) + } + } +} + func TestFormatJSON(t *testing.T) { - r := ProbeResult{ + r := runners.ProbeResult{ Endpoint: "http://x/v1", Model: "m", Enabled: true, - Reachable: true, HTTPStatus: 200, LatencyMS: 42, - ModelsSeen: []string{"m", "other"}, ModelMatch: true, + Status: runners.ProbeHealthy, HTTPStatus: 200, LatencyMS: 42, + ModelsSeen: []string{"m", "other"}, } out, err := formatJSON(r) if err != nil { @@ -263,10 +91,24 @@ func TestFormatJSON(t *testing.T) { if parsed["endpoint"] != "http://x/v1" { t.Errorf("endpoint: got %v, want http://x/v1", parsed["endpoint"]) } - if parsed["model_match"] != true { - t.Errorf("model_match: got %v, want true", parsed["model_match"]) + if parsed["status"] != "healthy" { + t.Errorf("status: got %v, want healthy", parsed["status"]) } if parsed["http_status"].(float64) != 200 { t.Errorf("http_status: got %v, want 200", parsed["http_status"]) } } + +func TestFormatJSON_ModelsSeenEmptyNotNull(t *testing.T) { + r := runners.ProbeResult{ + Endpoint: "http://x/v1", Model: "m", Enabled: true, + Status: runners.ProbeUnreachable, ModelsSeen: []string{}, + } + out, err := formatJSON(r) + if err != nil { + t.Fatalf("formatJSON err: %v", err) + } + if !strings.Contains(string(out), `"models_seen": []`) { + t.Errorf("want models_seen: [] (not null). Got:\n%s", out) + } +} diff --git a/src/runners/local.go b/src/runners/local.go index 8cd5719..1c59d94 100644 --- a/src/runners/local.go +++ b/src/runners/local.go @@ -64,27 +64,19 @@ func (r *LocalRunner) Available() bool { if !LocalEnabled() { return false } - endpoint := LocalEndpoint() - req, err := http.NewRequest(http.MethodGet, endpoint+"/models", nil) - if err != nil { - localDebugf("local runner probe: building request for %s: %v", endpoint, err) - return false - } - if key := LocalAPIKey(); key != "" { - req.Header.Set("Authorization", "Bearer "+key) - } - client := &http.Client{Timeout: 3 * time.Second} - resp, err := client.Do(req) - if err != nil { - localDebugf("local runner probe: %s unreachable: %v", endpoint, err) - return false - } - defer resp.Body.Close() - if resp.StatusCode >= 400 { - localDebugf("local runner probe: %s returned HTTP %d (check DEVKIT_LOCAL_API_KEY / endpoint path)", endpoint, resp.StatusCode) - return false + result := Probe(context.Background(), ProbeConfig{ + Enabled: true, + Endpoint: LocalEndpoint(), + Model: LocalModel(), + APIKey: LocalAPIKey(), + Timeout: 3 * time.Second, + }) + switch result.Status { + case ProbeHealthy, ProbeModelMissing: + return true } - return true + localDebugf("local runner probe: %s status=%s http=%d err=%s", result.Endpoint, result.Status, result.HTTPStatus, result.ErrorMsg) + return false } func (r *LocalRunner) Run(ctx context.Context, prompt string, opts RunOpts) (RunResult, error) { diff --git a/src/runners/local_test.go b/src/runners/local_test.go index 61e1050..86ad1ab 100644 --- a/src/runners/local_test.go +++ b/src/runners/local_test.go @@ -44,11 +44,14 @@ func TestLocalRunner_Available_Gating(t *testing.T) { if !tt.noServer && tt.enabled == "1" { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(tt.serverCode) + if tt.serverCode == 200 { + io.WriteString(w, `{"data":[]}`) + } })) defer srv.Close() endpoint = srv.URL } - t.Setenv("DEVKIT_LOCAL_ENDPOINT", endpoint) + t.Setenv("DEVKIT_LOCAL_ENDPOINT", endpoint+"/v1") r := &LocalRunner{} if got := r.Available(); got != tt.want { diff --git a/src/runners/probe.go b/src/runners/probe.go new file mode 100644 index 0000000..8fb66c1 --- /dev/null +++ b/src/runners/probe.go @@ -0,0 +1,151 @@ +package runners + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "strings" + "time" +) + +// ProbeStatus is the high-level outcome of a /v1/models probe. It is the +// source of truth for pass/fail decisions; HTTPStatus stays on ProbeResult as +// diagnostic data. +type ProbeStatus string + +const ( + ProbeDisabled ProbeStatus = "disabled" + ProbeUnreachable ProbeStatus = "unreachable" + ProbeEndpointError ProbeStatus = "endpoint_error" + ProbeInvalidResponse ProbeStatus = "invalid_response" + ProbeModelMissing ProbeStatus = "model_missing" + ProbeHealthy ProbeStatus = "healthy" +) + +type ProbeConfig struct { + Enabled bool + Endpoint string + Model string + APIKey string + Timeout time.Duration +} + +type ProbeResult struct { + Endpoint string `json:"endpoint"` + Model string `json:"model"` + Enabled bool `json:"enabled"` + Status ProbeStatus `json:"status"` + HTTPStatus int `json:"http_status"` + LatencyMS int64 `json:"latency_ms"` + ModelsSeen []string `json:"models_seen"` + ErrorMsg string `json:"error,omitempty"` + Hint string `json:"hint,omitempty"` +} + +// Probe performs an OpenAI-compatible /v1/models health check and reports the +// outcome as a ProbeStatus. It is used by both the `devkit-engine probe-local` +// CLI and LocalRunner.Available so the two cannot drift on reachability logic. +func Probe(ctx context.Context, cfg ProbeConfig) ProbeResult { + r := ProbeResult{ + Endpoint: cfg.Endpoint, + Model: cfg.Model, + Enabled: cfg.Enabled, + Status: ProbeDisabled, + ModelsSeen: []string{}, + } + if !cfg.Enabled { + return r + } + + reqCtx, cancel := context.WithTimeout(ctx, cfg.Timeout) + defer cancel() + + req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, cfg.Endpoint+"/models", nil) + if err != nil { + r.Status = ProbeUnreachable + r.ErrorMsg = fmt.Sprintf("building request: %v", err) + r.Hint = "check DEVKIT_LOCAL_ENDPOINT format — must be a valid URL ending in /v1" + return r + } + if cfg.APIKey != "" { + req.Header.Set("Authorization", "Bearer "+cfg.APIKey) + } + + client := &http.Client{Timeout: cfg.Timeout} + start := time.Now() + resp, err := client.Do(req) + r.LatencyMS = time.Since(start).Milliseconds() + if err != nil { + r.Status = ProbeUnreachable + r.ErrorMsg = err.Error() + r.Hint = unreachableHint(reqCtx, cfg.Timeout, err) + return r + } + defer resp.Body.Close() + r.HTTPStatus = resp.StatusCode + + if resp.StatusCode >= 400 { + body, _ := io.ReadAll(io.LimitReader(resp.Body, 256)) + r.Status = ProbeEndpointError + r.ErrorMsg = strings.TrimSpace(string(body)) + switch resp.StatusCode { + case 401, 403: + r.Hint = "check DEVKIT_LOCAL_API_KEY — endpoint requires auth" + case 404: + r.Hint = "check DEVKIT_LOCAL_ENDPOINT — must end in /v1 (some stacks need the suffix)" + default: + r.Hint = "endpoint returned an error — check server logs" + } + return r + } + + raw, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + r.Status = ProbeInvalidResponse + r.ErrorMsg = fmt.Sprintf("reading response: %v", err) + return r + } + + var parsed struct { + Data []struct { + ID string `json:"id"` + } `json:"data"` + } + if err := json.Unmarshal(raw, &parsed); err != nil { + r.Status = ProbeInvalidResponse + r.ErrorMsg = fmt.Sprintf("parsing /models response: %v", err) + r.Hint = "endpoint did not return OpenAI /models JSON — confirm it speaks the OpenAI spec" + return r + } + + match := false + for _, m := range parsed.Data { + r.ModelsSeen = append(r.ModelsSeen, m.ID) + if m.ID == cfg.Model { + match = true + } + } + if match { + r.Status = ProbeHealthy + return r + } + r.Status = ProbeModelMissing + r.Hint = fmt.Sprintf("configured model %q not in /models — check DEVKIT_LOCAL_MODEL matches a name the server exposes", cfg.Model) + return r +} + +// unreachableHint returns an actionable hint string for a network-layer +// failure. The context is checked first so we can distinguish caller-cancel +// from probe-timeout; otherwise we fall back to a generic reachability hint. +func unreachableHint(ctx context.Context, timeout time.Duration, err error) string { + if errors.Is(err, context.DeadlineExceeded) || errors.Is(ctx.Err(), context.DeadlineExceeded) { + return fmt.Sprintf("endpoint did not respond within %s — raise DEVKIT_LOCAL_TIMEOUT or check server health", timeout) + } + if errors.Is(err, context.Canceled) || errors.Is(ctx.Err(), context.Canceled) { + return "probe canceled by caller before the endpoint responded" + } + return "endpoint unreachable — check it is running and DEVKIT_LOCAL_ENDPOINT is correct" +} diff --git a/src/runners/probe_test.go b/src/runners/probe_test.go new file mode 100644 index 0000000..f015db0 --- /dev/null +++ b/src/runners/probe_test.go @@ -0,0 +1,211 @@ +package runners + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +func TestProbe_Disabled(t *testing.T) { + cfg := ProbeConfig{ + Enabled: false, + Endpoint: "http://localhost:8080/v1", + Model: "test-model", + Timeout: 1 * time.Second, + } + got := Probe(context.Background(), cfg) + + if got.Status != ProbeDisabled { + t.Errorf("Status: got %q, want %q", got.Status, ProbeDisabled) + } + if got.HTTPStatus != 0 { + t.Errorf("HTTPStatus: got %d, want 0 (no request should fire)", got.HTTPStatus) + } + if got.ModelsSeen == nil { + t.Errorf("ModelsSeen: got nil, want [] (disabled should still init slice)") + } +} + +func TestProbe_Healthy(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/v1/models" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"data":[{"id":"gemma-4-26b-a4b"},{"id":"mistral-small-4"}]}`) + })) + defer srv.Close() + + cfg := ProbeConfig{ + Enabled: true, + Endpoint: srv.URL + "/v1", + Model: "gemma-4-26b-a4b", + Timeout: 2 * time.Second, + } + got := Probe(context.Background(), cfg) + + if got.Status != ProbeHealthy { + t.Fatalf("Status: got %q, want %q (err=%q)", got.Status, ProbeHealthy, got.ErrorMsg) + } + if got.HTTPStatus != 200 { + t.Errorf("HTTPStatus: got %d, want 200", got.HTTPStatus) + } + wantModels := []string{"gemma-4-26b-a4b", "mistral-small-4"} + if len(got.ModelsSeen) != len(wantModels) { + t.Fatalf("ModelsSeen len: got %d, want %d", len(got.ModelsSeen), len(wantModels)) + } + for i, m := range wantModels { + if got.ModelsSeen[i] != m { + t.Errorf("ModelsSeen[%d]: got %q, want %q", i, got.ModelsSeen[i], m) + } + } +} + +func TestProbe_ModelMissing(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, `{"data":[{"id":"something-else"}]}`) + })) + defer srv.Close() + + got := Probe(context.Background(), ProbeConfig{ + Enabled: true, Endpoint: srv.URL + "/v1", Model: "not-there", Timeout: 2 * time.Second, + }) + + if got.Status != ProbeModelMissing { + t.Fatalf("Status: got %q, want %q", got.Status, ProbeModelMissing) + } + if got.Hint == "" { + t.Errorf("Hint: got empty, want actionable text about DEVKIT_LOCAL_MODEL") + } +} + +func TestProbe_Unauthorized(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + fmt.Fprint(w, `{"error":"unauthorized"}`) + })) + defer srv.Close() + + got := Probe(context.Background(), ProbeConfig{ + Enabled: true, Endpoint: srv.URL + "/v1", Model: "m", Timeout: 2 * time.Second, + }) + + if got.Status != ProbeEndpointError { + t.Errorf("Status: got %q, want %q on 401", got.Status, ProbeEndpointError) + } + if got.HTTPStatus != 401 { + t.Errorf("HTTPStatus: got %d, want 401", got.HTTPStatus) + } + if !strings.Contains(got.Hint, "DEVKIT_LOCAL_API_KEY") { + t.Errorf("Hint: got %q, want mention of DEVKIT_LOCAL_API_KEY", got.Hint) + } +} + +func TestProbe_NotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.NotFound(w, r) + })) + defer srv.Close() + + got := Probe(context.Background(), ProbeConfig{ + Enabled: true, Endpoint: srv.URL + "/wrong", Model: "m", Timeout: 2 * time.Second, + }) + + if got.Status != ProbeEndpointError { + t.Errorf("Status: got %q, want %q on 404", got.Status, ProbeEndpointError) + } + if got.HTTPStatus != 404 { + t.Errorf("HTTPStatus: got %d, want 404", got.HTTPStatus) + } + if !strings.Contains(got.Hint, "/v1") { + t.Errorf("Hint: got %q, want mention of /v1 suffix", got.Hint) + } +} + +func TestProbe_ServerError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"error":"boom"}`) + })) + defer srv.Close() + + got := Probe(context.Background(), ProbeConfig{ + Enabled: true, Endpoint: srv.URL + "/v1", Model: "m", Timeout: 2 * time.Second, + }) + + if got.Status != ProbeEndpointError { + t.Errorf("Status: got %q, want %q on 500", got.Status, ProbeEndpointError) + } + if got.HTTPStatus != 500 { + t.Errorf("HTTPStatus: got %d, want 500", got.HTTPStatus) + } + if !strings.Contains(got.Hint, "server logs") { + t.Errorf("Hint: got %q, want mention of server logs (default 5xx branch)", got.Hint) + } +} + +func TestProbe_Timeout(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(300 * time.Millisecond) + fmt.Fprint(w, `{"data":[]}`) + })) + defer srv.Close() + + got := Probe(context.Background(), ProbeConfig{ + Enabled: true, Endpoint: srv.URL + "/v1", Model: "m", Timeout: 50 * time.Millisecond, + }) + + if got.Status != ProbeUnreachable { + t.Errorf("Status: got %q, want %q on timeout", got.Status, ProbeUnreachable) + } + if !strings.Contains(got.Hint, "did not respond within") { + t.Errorf("Hint: got %q, want deadline-exceeded wording", got.Hint) + } +} + +func TestProbe_CallerCanceled(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-r.Context().Done() + })) + defer srv.Close() + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(30 * time.Millisecond) + cancel() + }() + + got := Probe(ctx, ProbeConfig{ + Enabled: true, Endpoint: srv.URL + "/v1", Model: "m", Timeout: 5 * time.Second, + }) + + if got.Status != ProbeUnreachable { + t.Errorf("Status: got %q, want %q on cancel", got.Status, ProbeUnreachable) + } + if !strings.Contains(got.Hint, "canceled by caller") { + t.Errorf("Hint: got %q, want caller-canceled wording", got.Hint) + } +} + +func TestProbe_BadJSON(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, `not json at all`) + })) + defer srv.Close() + + got := Probe(context.Background(), ProbeConfig{ + Enabled: true, Endpoint: srv.URL + "/v1", Model: "m", Timeout: 2 * time.Second, + }) + + if got.Status != ProbeInvalidResponse { + t.Errorf("Status: got %q, want %q on invalid JSON", got.Status, ProbeInvalidResponse) + } + if !strings.Contains(got.ErrorMsg, "parsing") { + t.Errorf("ErrorMsg: got %q, want mention of parse error", got.ErrorMsg) + } +} From 3aa32cc5fcf7443c158c4cde8ad2366688d035a6 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Fri, 17 Apr 2026 17:08:55 -0400 Subject: [PATCH 2/3] docs(readme): remove LocalAI double-listing from local-models examples LocalAI appeared in both the single-model and multi-model router example rows. Move it to the multi-model row only and clarify that it supports either mode depending on how it is configured. --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 6b0e4a1..c3bc489 100644 --- a/README.md +++ b/README.md @@ -133,7 +133,7 @@ Port 8080 is the llama.cpp convention; llama-swap reuses it since it fronts llam ### Examples -Single model, no router (llama-server, LM Studio, LocalAI, vLLM, SGLang): +Single model, no router (llama-server, LM Studio, vLLM, SGLang): ```bash export DEVKIT_LOCAL_ENABLED=1 @@ -142,7 +142,7 @@ export DEVKIT_LOCAL_MODEL= devkit-engine probe-local ``` -Multi-model via router (Ollama, llama-swap, LocalAI): +Multi-model via router (Ollama, llama-swap, LocalAI — LocalAI supports either mode depending on how it's configured): ```bash export DEVKIT_LOCAL_ENABLED=1 From d9b3848e0b2be33b94650040c1fb6da9afeed87b Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Fri, 17 Apr 2026 17:22:12 -0400 Subject: [PATCH 3/3] fix(runners): address second-pass tri-review blockers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bi-tier tri-review (smart codex + fast haiku + direct verification — gemini unavailable under issue #95 guard-cascade bug) reached consensus on 5 real blockers. Applied all: - probe.go unreachableHint: drop the "raise DEVKIT_LOCAL_TIMEOUT" wording. That env var is only read by LocalRunner.Run, not Probe; the hint lied. Rephrased to point at server health + DEVKIT_LOCAL_ENDPOINT. - probe.go endpoint normalization: strings.TrimRight("/") before appending "/models" so users with a trailing slash on DEVKIT_LOCAL_ENDPOINT don't get http://host/v1//models. - probe.go unreachableHint: add a doc comment explaining the intentional `err || ctx.Err()` OR pattern so it survives future "simplification". - probe_local.go + README.md: update the exit-code contract to state that exit 0 covers both healthy and disabled (the code already did this; the docs lied). - local_test.go Available_Gating: split the 200 case into "model missing" (empty /data) and "model present" (matching DEVKIT_LOCAL_MODEL). Without the second case a switch inversion at local.go:75 would pass tests. - probe_test.go TestProbe_CallerCanceled: replace the goroutine + Sleep(30ms) race with a `started` channel the handler closes when it's in flight; the test canceller blocks on receive. Deterministic, faster, not CI-flaky. Skipped: the pr-review-toolkit findings both reviewers independently classified as padding (cfg.Timeout=0-infinite-wait premise is wrong per Go context semantics; doubled context+client timeout is redundant not a bug; body ReadAll err discard + missing Hint on 2xx-ReadAll branch are inconsistencies; "only default branch logs" is wrong as stated — no default branch exists; bare sentinel fragility is hypothetical). --- README.md | 2 +- src/cmd/probe_local.go | 3 ++- src/runners/local_test.go | 22 ++++++++++++---------- src/runners/probe.go | 9 +++++++-- src/runners/probe_test.go | 4 +++- 5 files changed, 25 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index c3bc489..acbd08f 100644 --- a/README.md +++ b/README.md @@ -153,7 +153,7 @@ devkit-engine probe-local ### Verifying the setup -`devkit-engine probe-local` calls `$DEVKIT_LOCAL_ENDPOINT/models`, reports reachability and latency, and checks whether your configured model is present in the server's model list. Exit 0 on healthy, 1 otherwise. Add `--json` for structured output. +`devkit-engine probe-local` calls `$DEVKIT_LOCAL_ENDPOINT/models`, reports reachability and latency, and checks whether your configured model is present in the server's model list. Exit 0 on healthy (or when the local runner is disabled), 1 otherwise. Add `--json` for structured output. ### Limits diff --git a/src/cmd/probe_local.go b/src/cmd/probe_local.go index 80ea408..f5523db 100644 --- a/src/cmd/probe_local.go +++ b/src/cmd/probe_local.go @@ -67,7 +67,8 @@ var probeLocalCmd = &cobra.Command{ Long: `Probe the OpenAI-compatible endpoint configured via DEVKIT_LOCAL_* env vars. Reports endpoint, model, reachability, and whether the configured model is -present in the server's /v1/models response. Exit 0 on healthy, 1 otherwise.`, +present in the server's /v1/models response. Exit 0 on healthy or when the +local runner is disabled (DEVKIT_LOCAL_ENABLED != 1); exit 1 otherwise.`, // Override root's PersistentPreRunE — this probe doesn't need a git repo or DB. PersistentPreRunE: func(cmd *cobra.Command, args []string) error { return nil }, SilenceUsage: true, diff --git a/src/runners/local_test.go b/src/runners/local_test.go index 86ad1ab..f28f439 100644 --- a/src/runners/local_test.go +++ b/src/runners/local_test.go @@ -25,17 +25,19 @@ func TestLocalRunner_Available_Gating(t *testing.T) { name string enabled string serverCode int + body string noServer bool want bool }{ - {"env unset", "", 200, false, false}, - {"env=0", "0", 200, false, false}, - {"env=1 no server", "1", 0, true, false}, - {"env=1 server 200", "1", 200, false, true}, - {"env=1 server 401", "1", 401, false, false}, - {"env=1 server 404", "1", 404, false, false}, - {"env=1 server 500", "1", 500, false, false}, - {"env=1 server 503", "1", 503, false, false}, + {"env unset", "", 200, "", false, false}, + {"env=0", "0", 200, "", false, false}, + {"env=1 no server", "1", 0, "", true, false}, + {"env=1 200 model missing", "1", 200, `{"data":[]}`, false, true}, + {"env=1 200 model present", "1", 200, `{"data":[{"id":"qwen3:32b"}]}`, false, true}, + {"env=1 server 401", "1", 401, "", false, false}, + {"env=1 server 404", "1", 404, "", false, false}, + {"env=1 server 500", "1", 500, "", false, false}, + {"env=1 server 503", "1", 503, "", false, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -44,8 +46,8 @@ func TestLocalRunner_Available_Gating(t *testing.T) { if !tt.noServer && tt.enabled == "1" { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(tt.serverCode) - if tt.serverCode == 200 { - io.WriteString(w, `{"data":[]}`) + if tt.body != "" { + io.WriteString(w, tt.body) } })) defer srv.Close() diff --git a/src/runners/probe.go b/src/runners/probe.go index 8fb66c1..7f5d8ac 100644 --- a/src/runners/probe.go +++ b/src/runners/probe.go @@ -63,7 +63,8 @@ func Probe(ctx context.Context, cfg ProbeConfig) ProbeResult { reqCtx, cancel := context.WithTimeout(ctx, cfg.Timeout) defer cancel() - req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, cfg.Endpoint+"/models", nil) + endpoint := strings.TrimRight(cfg.Endpoint, "/") + req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, endpoint+"/models", nil) if err != nil { r.Status = ProbeUnreachable r.ErrorMsg = fmt.Sprintf("building request: %v", err) @@ -140,9 +141,13 @@ func Probe(ctx context.Context, cfg ProbeConfig) ProbeResult { // unreachableHint returns an actionable hint string for a network-layer // failure. The context is checked first so we can distinguish caller-cancel // from probe-timeout; otherwise we fall back to a generic reachability hint. +// unreachableHint returns an actionable hint string for a network-layer +// failure. The `err || ctx.Err()` OR pattern is intentional — either source +// may carry the deadline/cancel signal depending on where the cancel races +// the transport; keep both checks. func unreachableHint(ctx context.Context, timeout time.Duration, err error) string { if errors.Is(err, context.DeadlineExceeded) || errors.Is(ctx.Err(), context.DeadlineExceeded) { - return fmt.Sprintf("endpoint did not respond within %s — raise DEVKIT_LOCAL_TIMEOUT or check server health", timeout) + return fmt.Sprintf("endpoint did not respond within %s — check server health or confirm DEVKIT_LOCAL_ENDPOINT points to a running server", timeout) } if errors.Is(err, context.Canceled) || errors.Is(ctx.Err(), context.Canceled) { return "probe canceled by caller before the endpoint responded" diff --git a/src/runners/probe_test.go b/src/runners/probe_test.go index f015db0..7e6689c 100644 --- a/src/runners/probe_test.go +++ b/src/runners/probe_test.go @@ -169,14 +169,16 @@ func TestProbe_Timeout(t *testing.T) { } func TestProbe_CallerCanceled(t *testing.T) { + started := make(chan struct{}) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + close(started) <-r.Context().Done() })) defer srv.Close() ctx, cancel := context.WithCancel(context.Background()) go func() { - time.Sleep(30 * time.Millisecond) + <-started cancel() }()