Probe-local tri-review follow-ups: Status enum + shared probe + hint/test/contract fixes - #97
Merged
Merged
Conversation
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.
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.
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Applies all follow-ups from the tri-review of just-merged PR #93 (probe-local feature), plus five additional blockers surfaced by a second-pass tri-review (smart codex + fast haiku + direct verification) on this branch.
Commit 1:
refactor(runners): consolidate probe core with Status enum(441dffc)cmdintorunners/probe.go;LocalRunner.Availableanddevkit-engine probe-localnow share one reachability path (addresses Gemini's tri-review ask — no more drift risk).ProbeStatusenum (disabled/unreachable/endpoint_error/invalid_response/model_missing/healthy) replacing the oldReachable+HTTPStatus+ModelMatchbooleans. Removes the 2xx-with-bad-JSON ambiguity.ProbeConfig/ProbeResultfromcmd(moved torunners, where they have a real caller).ModelsSeeninit'd to[]string{}— JSON emits[]notnull.unreachableHintdistinguishescontext.DeadlineExceeded/context.Canceled/ generic.Commit 2:
docs(readme): remove LocalAI double-listing(3aa32cc)Commit 3:
fix(runners): address second-pass tri-review blockers(d9b3848)Five findings that reached consensus across both reviewer tiers:
DEVKIT_LOCAL_TIMEOUT" — env var is only read byLocalRunner.Run, notProbeAvailable_Gatingtest never exercisedProbeHealthy→true— a switch inversion would passTestProbe_CallerCanceledused goroutine+Sleep(30ms)+cancel — flaky under CI loadstarted, canceller blocks on receivecfg.Endpoint+"/models"didn't normalize trailing slashstrings.TrimRight(endpoint, "/")Skipped (both reviewers classified as padding or wrong premise):
cfg.Timeout=0-infinite-wait(Gocontext.WithTimeout(ctx, 0)creates an already-expired context, not infinite wait), doubled context+client timeout (redundant not broken), discarded body-ReadAll err on 4xx/5xx (lost diagnostic not broken classification), "logs only default branch" (no default branch exists — fires on all non-healthy cases), bare sentinel fragility (hypothetical).Known workflow gap
The
/devkit:tri-reviewpass on this branch surfaced a recurrence of issue #95: thegemini-rescuegeneral-tier subagent's internal Bash call togemini-companion.mjsis still blocked bydevkit-guardduring prompt steps, even after the Agent/Task dispatch fix in v2.1.25. Ran bi-tier (smart+fast) with direct verification as the fallback.Test plan
go build ./...cleango test ./...→ 387 passed (was 373 on PR feat: local runner health probe #93, +14 from new enum-based probe tests + format tests + new Available subtest + canceled test + models_seen-empty-not-null)gofmt -lclean on all changed filesgo vet ./...clean