fix(probe): portless services report "unknown", never "down"

[chazmaniandinkle]

The defect

NodeHealth.Probe() in internal/engine/node_probe.go built every probe as:

url := fmt.Sprintf("http://localhost:%d%s", svc.Port, svc.Health)

HTTP only, hardcoded, and entirely kind-blind. Before this change:

$ grep -nE "Kind|observed|external" internal/engine/node_probe.go
$          # nothing. not one hit.

ServiceKindObserved / ServiceKindExternal are honoured at serve_services.go:149 for the supervisor projection — but the prober never learned they existed. A service with no port was probed at http://localhost:0/..., failed to connect, and was reported permanently "down" while running perfectly.

Why it matters more than a wrong string

A service that is always red trains the operator to ignore the health surface.

That is not hypothetical here — it is the exact silent-dashboard failure this codebase has already suffered (dome :8801; the doc_activity bus dying unnoticed). A health surface with a known-bogus red tile is worse than one with an acknowledged gap, because the gap is honest and the red tile is a lie you learn to skip over.

The fix — Option A

When a service has Port == 0, or its EffectiveKind() is observed/external with an empty Health path: skip the HTTP probe and report "unknown" — never "down".

unknown is absence of evidence. down is a positive claim of failure we had not earned.

Extracted as a single predicate, probeable(svc ServiceDef) bool, so the rule lives in one place.

Also fixed: probeService() on the cogos node status CLI path had the identical hardcoded-HTTP defect and is now routed through the same predicate. unknown renders dim (\033[90m) rather than falling into the red default: branch — the point of the change is to stop painting things red that aren't broken.

Rejected alternatives

• An exec-probe field on ServiceDef. Having the kernel execute manifest-supplied command strings is a new privilege surface. That deserves its own ADR and threat model, not a slipstream into a bugfix PR.
• An HTTP shim wrapping osascript. This adds a daemon whose own liveness must then be monitored — you have solved "is the service up?" by introducing a second thing that can be silently down. It directly contradicts the standing daemonless / in-process preference.

Verified, not assumed

The brief asked me to check rather than assume that unknown is already falsy downstream. I did:

• projectService() running — serve_services.go:162 is running := h.Status == "healthy" || h.Status == "degraded". "unknown" matches neither, so it is already falsy. Confirmed by reading, then pinned by TestProjectService_UnknownIsNotRunning so a future edit to that predicate can't silently regress it.
• /v1/services JSON view — healthV is constructed whenever h.Status != "", so unknown surfaces as a real health object with its endpoint and probed_at rather than vanishing into a null. Asserted in the same test.
• Counts() — increments only on "healthy", so unknown cannot inflate the healthy count while still being included in the total. Asserted by TestNodeHealth_UnknownIsNotCountedHealthy.
• Severity/render paths — node_cmd.go was the only place where an unrecognised status fell through to a red default:; that is now handled explicitly.

One incidental correctness note: the self-skip was svc.Port == selfPort, which would have skipped a portless service outright if selfPort were ever 0. It is now guarded with svc.Port != 0 &&, so portless services are always reported rather than silently omitted.

Tests — including a negative control

ServiceKindObserved previously had 9 uses, all in tests, zero in any live manifest. This is its first production-shaped use, so the tests carry the weight.

The negative control is TestProbe_PortlessObservedServiceIsNotDown. It asserts what the status is NOT, because "not down" is the bug. I verified it actually bites by reverting the probe logic while keeping the test:

--- FAIL: TestProbe_PortlessObservedServiceIsNotDown
    portless observed service reported "down"; this is the exact defect
    — a running service painted permanently red
--- FAIL: TestNodeHealth_UnknownIsNotCountedHealthy
    summary[portless] = "down"; want "unknown"
--- FAIL: TestProbeService_PortlessReportsUnknown
    cogos node status reported a portless observed service as down

Restored, all pass. A test that never fails against the unfixed code is decoration.

The regression guard is TestProbe_NormalHTTPServiceStillProbes: real httptest servers covering healthy (200), degraded (500), and down (a bound-then-closed port), plus an observed service that does declare a health path and therefore must still be probed normally. The down case is load-bearing — it proves the fix did not blanket-suppress genuine failures, which would have traded one silent dashboard for another.

Plus TestProbe_PortlessKindsAllReportUnknown across every unprobeable shape and TestProbeable_Table pinning the predicate directly.

Suite status

$ go build ./...
$ go test ./internal/engine/ -count=1
ok  github.com/myrgic/cogos/internal/engine   65.961s

Full pre-existing suite passes. No regressions. gofmt clean on all three touched files.

Branched from origin/main (a8ff58d) — deliberately not stacked on doctor-loose-secrets (#576), so the two land independently.

[github-actions]

🤖 cog-review — request_changes (head 434bac3)

The PR correctly fixes the core defect described: portless services and observed/external services with an empty Health path are now reported "unknown" instead of "down", and this is done via a single well-tested probeable() predicate shared by both the NodeHealth.Probe path and the cogos node status CLI path. All of the PR's downstream claims were independently verified by reading the code: projectService's running predicate treats "unknown" as falsy (serve_services.go:162), the /v1/services JSON view still surfaces an "unknown" health object rather than omitting it (serve_services.go:165-171), Counts() only increments on "healthy" (node_probe.go:132-137), and node_cmd.go's status renderer now handles "unknown" explicitly instead of falling into the red default case. The self-skip guard fix (svc.Port != 0 && svc.Port == selfPort) is also correct and matches the incidental-fix description. However, probeable() conflates ServiceKindExternal with ServiceKindObserved: it only exempts a kind from probing when Health == "", but the ServiceKind doc comment (node_manifest.go:24-25, pre-existing and undisturbed by this PR) states that "external" services are never probed by the kernel, unconditionally — not just when they lack a health path. The repo's own reference manifest (.cog/config/node/manifest.yaml) ships exactly this shape (kind: external, port set, health: /health), so an external service configured per that documented example will still get an HTTP probe issued against it and can be marked "down" for reasons having nothing to do with its actual health — reintroducing a variant of the bug this PR is fixing, just for a different sibling case. No test in the new suite exercises the external+health combination (only observed+health is asserted as "still probed"), so this gap wasn't caught by the PR's own verification process despite its stated goal of covering the whole class of unprobeable/misprobed shapes.

Confirmed findings (1):

• internal/engine/node_probe.go:42 — probeable() allows HTTP probing of ServiceKindExternal services whenever Health is set, contradicting the ServiceKind doc contract that external services are never probed.
  • Failure scenario: A manifest defines a service with kind: external, a nonzero port, and a health path (exactly as the repo's shipped reference manifest .cog/config/node/manifest.yaml does for its gateway service: port 18789, health: /health). probeable() returns true for it, so NodeHealth.Probe issues an HTTP GET to http://localhost:18789/health. Since the kernel never started this externally-managed process and has no guarantee it's reachable at that literal localhost path, the probe can fail and the service is reported "down" even though, per the documented external-kind contract, the kernel was never supposed to probe it at all — the same class of false-red-tile bug this PR fixes for portless services, left uncorrected for this sibling case.

Unverified notes:
none

---

This review was generated by an AI reviewer with review-only authority — it can approve or block, it cannot merge or close. Verdict basis: pr-review-rubric.md. The merge decision belongs to a human or their operator workflow.