diff --git a/apps/backend/internal/auth/httpmw/middleware_test.go b/apps/backend/internal/auth/httpmw/middleware_test.go index 6b2726eee8..9e683c0ae0 100644 --- a/apps/backend/internal/auth/httpmw/middleware_test.go +++ b/apps/backend/internal/auth/httpmw/middleware_test.go @@ -189,6 +189,12 @@ func TestEnabledModeAllowlistMatrix(t *testing.T) { }, {name: "office without bearer", method: http.MethodGet, path: "/api/v1/office/tasks/t1", blocked: true}, + { + // AC-14 (docs/specs/health-endpoint-version/spec.md): /health + // gained a version field, but /api/v1/system/info is not on the + // allowlist and must stay rejected — the change must not widen it. + name: "system info", method: http.MethodGet, path: "/api/v1/system/info", blocked: true, + }, {name: "tasks api", method: http.MethodGet, path: "/api/v1/tasks", blocked: true}, {name: "workspaces api", method: http.MethodGet, path: "/api/v1/workspaces", blocked: true}, {name: "plugin management", method: http.MethodGet, path: "/api/plugins", blocked: true}, diff --git a/apps/backend/internal/backendapp/health_test.go b/apps/backend/internal/backendapp/health_test.go new file mode 100644 index 0000000000..837d01e354 --- /dev/null +++ b/apps/backend/internal/backendapp/health_test.go @@ -0,0 +1,267 @@ +package backendapp + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + + authhttpmw "github.com/kandev/kandev/internal/auth/httpmw" + "github.com/kandev/kandev/internal/system/info" +) + +// setReadyForTest flips the package-level readiness flag consulted by +// healthHandler and restores its prior value on cleanup, so tests don't leak +// state into each other or into TestMain-driven suites. +func setReadyForTest(t *testing.T, value bool) { + t.Helper() + prev := ready.Load() + ready.Store(value) + t.Cleanup(func() { ready.Store(prev) }) +} + +// TestHealthHandlerReadyBodyIncludesVersion covers AC-1..4: once ready, the +// handler returns 200 with a version key equal to the configured build +// version, alongside the unchanged status/service/mode fields, and no other +// keys. +func TestHealthHandlerReadyBodyIncludesVersion(t *testing.T) { + gin.SetMode(gin.TestMode) + setReadyForTest(t, true) + + router := gin.New() + router.GET("/health", healthHandler(routeParams{version: "1.2.3"})) + + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, "/health", nil)) + + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", recorder.Code, http.StatusOK) + } + var body map[string]interface{} + if err := json.Unmarshal(recorder.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body["version"] != "1.2.3" { + t.Fatalf("version = %v, want 1.2.3", body["version"]) + } + if body["status"] != "ok" || body["service"] != "kandev" || body["mode"] != "websocket+http" { + t.Fatalf("unexpected ready body: %#v", body) + } + if len(body) != 4 { + t.Fatalf("ready body keys = %#v, want exactly status/service/mode/version", body) + } +} + +// TestHealthHandlerStartingBodyIncludesVersion covers AC-5..7: before ready, +// the handler still returns the version alongside status/service, with no +// other keys (mode is ready-path only). +func TestHealthHandlerStartingBodyIncludesVersion(t *testing.T) { + gin.SetMode(gin.TestMode) + setReadyForTest(t, false) + + router := gin.New() + router.GET("/health", healthHandler(routeParams{version: "1.2.3"})) + + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, "/health", nil)) + + if recorder.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", recorder.Code, http.StatusServiceUnavailable) + } + var body map[string]interface{} + if err := json.Unmarshal(recorder.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body["version"] != "1.2.3" { + t.Fatalf("version = %v, want 1.2.3", body["version"]) + } + if body["status"] != "starting" || body["service"] != "kandev" { + t.Fatalf("unexpected starting body: %#v", body) + } + if len(body) != 3 { + t.Fatalf("starting body keys = %#v, want exactly status/service/version", body) + } +} + +// TestHealthHandlerDesktopTokenHeaderOnlyOnReadyPath guards a pre-existing +// behavior the spec restates as a must-not-regress ("## What": the desktop +// health-token header SHALL continue to be set on the 200 path only, +// unchanged) but that had no direct test before this change extracted the +// handler body into healthHandler. Confirms the refactor didn't move the +// header write outside its original branch. +func TestHealthHandlerDesktopTokenHeaderOnlyOnReadyPath(t *testing.T) { + gin.SetMode(gin.TestMode) + t.Setenv(desktopHealthTokenEnv, " desktop-token ") + + router := gin.New() + router.GET("/health", healthHandler(routeParams{version: "1.2.3"})) + + setReadyForTest(t, false) + startingRec := httptest.NewRecorder() + router.ServeHTTP(startingRec, httptest.NewRequest(http.MethodGet, "/health", nil)) + if got := startingRec.Header().Get(desktopHealthTokenHeader); got != "" { + t.Fatalf("starting-path %s header = %q, want absent", desktopHealthTokenHeader, got) + } + + setReadyForTest(t, true) + readyRec := httptest.NewRecorder() + router.ServeHTTP(readyRec, httptest.NewRequest(http.MethodGet, "/health", nil)) + if got := readyRec.Header().Get(desktopHealthTokenHeader); got != "desktop-token" { + t.Fatalf("ready-path %s header = %q, want desktop-token", desktopHealthTokenHeader, got) + } +} + +// TestHealthHandlerVersionMatchesSystemInfoVersion covers AC-10: /health and +// /api/v1/system/info are fed the exact same build-version string +// (backendapp.Version, sampled after setBuildInfo — see main.go:801,1830), so +// this pins that both handlers surface it byte-identically rather than each +// having its own notion of "version". +func TestHealthHandlerVersionMatchesSystemInfoVersion(t *testing.T) { + gin.SetMode(gin.TestMode) + setReadyForTest(t, true) + + const sameProcessVersion = "9.9.9-test" + + healthRouter := gin.New() + healthRouter.GET("/health", healthHandler(routeParams{version: sameProcessVersion})) + healthRec := httptest.NewRecorder() + healthRouter.ServeHTTP(healthRec, httptest.NewRequest(http.MethodGet, "/health", nil)) + var healthBody map[string]interface{} + if err := json.Unmarshal(healthRec.Body.Bytes(), &healthBody); err != nil { + t.Fatalf("decode health body: %v", err) + } + + infoSvc := info.NewService(sameProcessVersion, "commit", "buildtime") + infoRouter := gin.New() + infoRouter.GET("/info", info.Handler(infoSvc)) + infoRec := httptest.NewRecorder() + infoRouter.ServeHTTP(infoRec, httptest.NewRequest(http.MethodGet, "/info", nil)) + var infoBody info.Response + if err := json.Unmarshal(infoRec.Body.Bytes(), &infoBody); err != nil { + t.Fatalf("decode info body: %v", err) + } + + if healthBody["version"] != infoBody.Version { + t.Fatalf("health version = %v, system/info version = %v, want equal", healthBody["version"], infoBody.Version) + } +} + +// TestSetBuildInfoVersionDefaultAndInjection covers AC-8, AC-9, and AC-11: an +// unstamped build keeps the compiled-in "dev" default, a non-empty ldflag +// value overwrites it, and an empty injected value is ignored rather than +// clearing the version to "". +func TestSetBuildInfoVersionDefaultAndInjection(t *testing.T) { + prevVersion, prevCommit, prevBuildTime := Version, Commit, BuildTime + t.Cleanup(func() { Version, Commit, BuildTime = prevVersion, prevCommit, prevBuildTime }) + + // AC-8: the compiled-in default (before this test or anything else + // mutates the package var) must actually be "dev". Asserted against + // prevVersion, captured above before any mutation in this test — + // asserting post-mutation would only prove setBuildInfo doesn't clobber + // an already-set value, not that the real default is "dev". + if prevVersion != "dev" { + t.Fatalf("compiled-in default Version = %q, want dev", prevVersion) + } + + Version = "dev" + setBuildInfo(BuildInfo{}) + if Version != "dev" { + t.Fatalf("Version after empty BuildInfo = %q, want dev default retained", Version) + } + if Version == "" { + t.Fatal("Version must never become empty") + } + + setBuildInfo(BuildInfo{Version: "2.3.4"}) + if Version != "2.3.4" { + t.Fatalf("Version after injected BuildInfo = %q, want 2.3.4", Version) + } + + setBuildInfo(BuildInfo{Version: ""}) + if Version != "2.3.4" { + t.Fatalf("Version after empty ldflag = %q, want prior value 2.3.4 retained", Version) + } +} + +// TestHealthHandlerFallsBackToPackageVersionWhenParamEmpty covers AC-11 +// against the production wiring gap: every other test in this file passes an +// explicit routeParams{version: "..."}, so none of them would notice if the +// one-line "version: Version" field assignment that wires routeParams.version +// to the package-level Version var (main.go, in the registerRoutes call built +// by run()) were ever deleted — /health would then silently serve +// "version":"" in production. Standing up run()'s full DI graph just to +// exercise that single field is out of proportion to this change (see the +// spec's Implementation Notes). Instead, healthHandler falls back to the +// package-level Version whenever routeParams.version is unset, so the never- +// empty guarantee holds at the handler layer regardless of what the caller +// wires up. +func TestHealthHandlerFallsBackToPackageVersionWhenParamEmpty(t *testing.T) { + gin.SetMode(gin.TestMode) + + prevVersion := Version + t.Cleanup(func() { Version = prevVersion }) + Version = "9.9.9-fallback-test" + + router := gin.New() + router.GET("/health", healthHandler(routeParams{})) + + for _, tc := range []struct { + name string + ready bool + }{ + {name: "ready", ready: true}, + {name: "starting", ready: false}, + } { + t.Run(tc.name, func(t *testing.T) { + setReadyForTest(t, tc.ready) + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, "/health", nil)) + + var body map[string]interface{} + if err := json.Unmarshal(recorder.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body["version"] != "9.9.9-fallback-test" { + t.Fatalf("version = %v, want fallback to package-level Version %q", body["version"], "9.9.9-fallback-test") + } + }) + } +} + +// TestHealthHandlerAuthEnabledServesVersionWithoutCredential covers AC-12 and +// AC-13: with the auth feature flag on and no credential presented, /health +// must still return its full body (including version) rather than 401/403, +// and the readiness status code/field must be unchanged by auth being on. +func TestHealthHandlerAuthEnabledServesVersionWithoutCredential(t *testing.T) { + gin.SetMode(gin.TestMode) + setReadyForTest(t, true) + + svc := newSSOTestAuthService(t) + if _, _, err := svc.Setup(context.Background(), "admin@example.com", "adminpass123", "Admin", "", ""); err != nil { + t.Fatalf("Setup admin: %v", err) + } + + router := gin.New() + router.Use(authhttpmw.Middleware(svc)) + router.GET("/health", healthHandler(routeParams{version: "1.2.3"})) + + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, "/health", nil)) + + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want %d (no auth challenge on /health)", recorder.Code, http.StatusOK) + } + var body map[string]interface{} + if err := json.Unmarshal(recorder.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body["version"] != "1.2.3" { + t.Fatalf("version = %v, want 1.2.3", body["version"]) + } + if body["status"] != "ok" { + t.Fatalf("status field = %v, want ok", body["status"]) + } +} diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index a4fda901ab..e75659d024 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -675,7 +675,7 @@ func registerRoutes(p routeParams) { // Before that, return 503 so callers (including the e2e fixture's // waitForHealth) keep polling instead of racing ahead and hitting // 404s on routes that aren't wired yet. - p.router.GET("/health", healthHandler()) + p.router.GET("/health", healthHandler(p)) // /api/v1/features is a public, unauthenticated read of the runtime // feature-flag map. The frontend SSR-fetches it once per page render to @@ -736,16 +736,39 @@ func registerRoutes(p routeParams) { } } -func healthHandler() gin.HandlerFunc { +// healthHandler serves GET /health. The response always carries the +// process's running version — in both the ready and not-ready bodies — so an +// operator can identify the build of a backend that is stuck starting, and so +// a monitor never needs a credential to read it (see docs/specs/ +// health-endpoint-version/spec.md). +func healthHandler(p routeParams) gin.HandlerFunc { + // p.version is normally the package-level Version wired in by run()'s + // registerRoutes call. Falling back here keeps the never-empty guarantee + // (AC-11) true regardless of that call-site wiring, since standing up + // run()'s full DI graph in a test just to exercise that one field + // assignment is out of proportion to this change. + version := p.version + if version == "" { + version = Version + } return func(c *gin.Context) { if !ready.Load() { - c.JSON(http.StatusServiceUnavailable, gin.H{"status": "starting", "service": "kandev"}) + c.JSON(http.StatusServiceUnavailable, gin.H{ + "status": "starting", + "service": "kandev", + "version": version, + }) return } if token := desktopHealthToken(); token != "" { c.Header(desktopHealthTokenHeader, token) } - c.JSON(http.StatusOK, gin.H{"status": "ok", "service": "kandev", "mode": "websocket+http"}) + c.JSON(http.StatusOK, gin.H{ + "status": "ok", + "service": "kandev", + "mode": "websocket+http", + "version": version, + }) } } diff --git a/apps/backend/internal/backendapp/server_test.go b/apps/backend/internal/backendapp/server_test.go index 23d68bf19f..56a39ee22d 100644 --- a/apps/backend/internal/backendapp/server_test.go +++ b/apps/backend/internal/backendapp/server_test.go @@ -46,7 +46,7 @@ func TestHealthHandlerEchoesConfiguredToken(t *testing.T) { t.Cleanup(func() { ready.Store(false) }) router := gin.New() - router.GET("/health", healthHandler()) + router.GET("/health", healthHandler(routeParams{})) response := httptest.NewRecorder() request := httptest.NewRequest(http.MethodGet, "/health", nil) router.ServeHTTP(response, request) diff --git a/docs/public/operations.md b/docs/public/operations.md index f8d3cff556..7c8520a997 100644 --- a/docs/public/operations.md +++ b/docs/public/operations.md @@ -67,10 +67,10 @@ curl -fsS http://127.0.0.1:38429/health After startup it returns HTTP 200 with: ```json -{"status":"ok","service":"kandev","mode":"websocket+http"} +{"status":"ok","service":"kandev","mode":"websocket+http","version":"1.2.3"} ``` -It returns HTTP 503 with `status: "starting"` until routes, the agent registry, and the listener are ready. The supplied Kubernetes probes use this endpoint. +It returns HTTP 503 with `status: "starting"` (plus the same `version`) until routes, the agent registry, and the listener are ready. The supplied Kubernetes probes use this endpoint. `/health` is unauthenticated even when auth is enabled, so it's also the credential-free way for monitoring to read the running version — no need to authenticate to **System > About** just to check what build is deployed. For application diagnostics, open **Settings > System > Status** or request: diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index cc05b35e6e..9fb7086a5c 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -50,6 +50,7 @@ Product-wide capabilities that are not tied to a single feature area. | [git-subprocess-admission](platform/git-subprocess-admission.md) | building | | [bounded-task-status-delivery](platform/bounded-task-status-delivery.md) | approved | | [diagnostic-logging](platform/diagnostic-logging.md) | approved | +| [health-endpoint-version](health-endpoint-version/spec.md) | building | ## tasks/ — task & workflow model diff --git a/docs/specs/health-endpoint-version/spec.md b/docs/specs/health-endpoint-version/spec.md new file mode 100644 index 0000000000..af87cbb421 --- /dev/null +++ b/docs/specs/health-endpoint-version/spec.md @@ -0,0 +1,411 @@ +--- +status: building +created: 2026-08-07 +owner: nova28 +--- + +# Health Endpoint — Surface the Running Version + +## Why + +`GET /health` is the endpoint every operator already polls. Kubernetes liveness and +readiness probes (`k8s/deployment.yaml:39,47`), the CLI (`apps/cli/src/health.ts`), +the Go launcher (`internal/launcher/health.go:44`), the Homebrew service wrapper +(`scripts/release/kandev.rb`), the Tauri desktop shell +(`apps/desktop/src-tauri/src/backend.rs:555`), and the Playwright fixture +(`apps/web/e2e/fixtures/backend.ts:461`) all hit it. It answers "is this process up?" +but not "*which build* is up?", so an operator watching a rollout, a bad canary, or a +stuck upgrade cannot tell from their monitoring which version is answering. + +The version is not reachable from an equivalent place today. `GET /api/v1/system/info` +does return it, but that route is **not** on the unauthenticated allowlist +(`internal/auth/httpmw/middleware.go:85-99`), while `/health` **is**. Once auth is +enabled, a monitoring system must be issued a credential purely to read a version +string. Adding the field to `/health` closes that gap for every unauthenticated prober +without weakening any existing boundary. + +**Correction to the originating request.** The request stated that `kandev_version` is +"already on `/system/info`". That is not accurate, and the difference matters because it +determines the field name this spec freezes. `GET /api/v1/system/info` returns a field +named **`version`** (`internal/system/info/info.go:19`), not `kandev_version`. The +identifier `kandev_version` does exist in this codebase, but for two unrelated things: +the `kandev_meta.kandev_version` database key that drives upgrade-backup safety +(`internal/persistence/meta.go:83`, ADR `0008-db-upgrade-safety.md`), and the +share-snapshot payload field (`internal/task/share/snapshot.go:19`). Both names +therefore had prior art. The name was resolved by explicit decision — see +[Decisions](#decisions). + +## Current State (verified 2026-08-07) + +There are **two** distinct health endpoints. This spec changes exactly one of them. + +| Endpoint | Registered at | Auth | Purpose | In scope | +|---|---|---|---|---| +| `GET /health` | `internal/backendapp/helpers.go:675` | Public (`middleware.go:87`) | Readiness probe for orchestrators, CLI, desktop, launcher | **Yes** | +| `GET /api/v1/system/health` | `internal/health/handler.go:13` | Authenticated | Diagnostics/issue list behind Settings > System > Status | No | +| `GET /api/v1/system/info` | `internal/system/system.go:175` | Authenticated | Build + runtime detail for System > About | No | +| WS action `health.check` | `internal/gateway/websocket/handler.go:74` | Session-bound | In-band WS liveness echo | No | + +The current `/health` handler in full (`helpers.go:675-684`): + +```go +p.router.GET("/health", func(c *gin.Context) { + if !ready.Load() { + c.JSON(http.StatusServiceUnavailable, gin.H{"status": "starting", "service": "kandev"}) + return + } + if token := desktopHealthToken(); token != "" { + c.Header(desktopHealthTokenHeader, token) + } + c.JSON(http.StatusOK, gin.H{"status": "ok", "service": "kandev", "mode": "websocket+http"}) +}) +``` + +`ready` is an `atomic.Bool` (`main.go:171`) flipped only after all routes are mounted, +the agent registry is seeded, and the listener is accepting. Before that the handler +returns 503 so probers keep polling instead of racing ahead onto unmounted routes. + +**The version value is already in scope at this exact line.** `routeParams` carries a +`version string` field (`helpers.go:545`), populated from the package-level `Version` at +`main.go:1830`, which is set by `setBuildInfo` from the ldflag-injected +`cmd/kandev/main.go:13`. It is already consumed nearby at `helpers.go:1180`. No new +plumbing, DI, or constructor change is required. + +`Version` defaults to the literal `"dev"` (`cmd/kandev/main.go:13`, +`internal/backendapp/main.go:129`), and `setBuildInfo` only overwrites it when the +injected value is non-empty (`main.go:251`). The value is therefore **never the +empty string** at runtime. + +**No test asserts the `/health` response body today.** A repo-wide search for the +current payload finds exactly two occurrences: the handler itself and a public doc +(`docs/public/operations.md:70`). This spec's acceptance criteria close that gap. + +## What + +- `GET /health` SHALL include the running Kandev version in its JSON response body. +- The field SHALL be named **`version`**, matching `GET /api/v1/system/info`. +- The field SHALL be present in **both** the ready (200) and not-ready (503) responses, + so an operator can identify the build of a backend that is stuck starting. +- The field value SHALL be the same string `GET /api/v1/system/info` reports as + `version` for the same running process. +- The field SHALL be served to unauthenticated callers, exactly as the rest of the + `/health` payload already is. This is a deliberate, accepted disclosure — see + [Security](#security-and-permissions). +- Every existing field (`status`, `service`, `mode`) SHALL retain its current name, + value, and semantics. This is a purely additive change. +- The existing HTTP status semantics (200 when ready, 503 while starting) SHALL be + unchanged. +- The desktop health-token response header SHALL continue to be set on the 200 path only, + unchanged. + +### API surface + +Ready (HTTP 200), current → new: + +```json +{"status":"ok","service":"kandev","mode":"websocket+http"} +{"status":"ok","service":"kandev","mode":"websocket+http","version":"1.2.3"} +``` + +Starting (HTTP 503), current → new: + +```json +{"status":"starting","service":"kandev"} +{"status":"starting","service":"kandev","version":"1.2.3"} +``` + +On an un-stamped local build the value is `"dev"`: + +```json +{"status":"ok","service":"kandev","mode":"websocket+http","version":"dev"} +``` + +## Acceptance Criteria + +Numbered, pass/fail, EARS-form. Each is directly testable against the handler. + +**Ready-path payload** + +1. WHEN a `GET /health` request is received AND the readiness flag is set, the backend + SHALL respond with HTTP 200 and a JSON body containing a `version` key. +2. WHEN a `GET /health` request is received AND the readiness flag is set, the backend + SHALL respond with a body whose `version` value equals the process's configured build + version string. +3. WHEN a `GET /health` request is received AND the readiness flag is set, the backend + SHALL respond with a body in which `status` equals `"ok"`, `service` equals + `"kandev"`, and `mode` equals `"websocket+http"`. +4. WHEN a `GET /health` request is received AND the readiness flag is set, the response + body SHALL contain exactly the keys `status`, `service`, `mode`, and `version` and no + others. + +**Starting-path payload** + +5. WHILE the readiness flag is unset, a `GET /health` request SHALL receive HTTP 503. +6. WHILE the readiness flag is unset, a `GET /health` request SHALL receive a JSON body + containing a `version` key whose value equals the process's configured build version + string. +7. WHILE the readiness flag is unset, a `GET /health` response body SHALL contain exactly + the keys `status`, `service`, and `version`, with `status` equal to `"starting"` and + `service` equal to `"kandev"`. + +**Value correctness** + +8. WHERE the binary is built without version ldflags, a `GET /health` response SHALL + report `version` as `"dev"`. +9. WHERE the binary is built with a version ldflag, a `GET /health` response SHALL report + `version` as that injected value. +10. The `version` value returned by `GET /health` SHALL be byte-identical to the + `version` value returned by `GET /api/v1/system/info` from the same running process. +11. A `GET /health` response SHALL NOT report `version` as an empty string, absent, or + JSON `null` under any readiness state. + +**Access and compatibility** + +12. WHEN authentication is enabled AND a `GET /health` request arrives with no credential, + the backend SHALL respond with the full body including `version`, and SHALL NOT + respond 401 or 403. +13. The change SHALL NOT alter the response status code, headers, or the `status` field + value for any readiness state, so that an existing prober asserting only HTTP status + or `status` continues to pass unmodified. +14. WHEN authentication is enabled AND a `GET /api/v1/system/info` request arrives with no + credential, the backend SHALL continue to reject it, confirming this change does not + widen the allowlist. + +**Documentation** + +15. `docs/public/operations.md` SHALL show the `/health` example body including the + `version` field, so the documented payload matches the served payload byte-for-byte. + +## Security and Permissions + +`/health` is on the unauthenticated allowlist (`internal/auth/httpmw/middleware.go:87`, +which carries a pinning test at `middleware_test.go:162`). Adding `version` therefore +publishes the exact running version to anyone who can reach the port. + +This is a **deliberate accepted tradeoff**, decided explicitly (see +[Decisions](#decisions)). The rationale: the stated goal is monitoring and alerting, and +external monitors are precisely the unauthenticated callers. Gating the field behind auth +would defeat the purpose for exactly the security-conscious deployments most likely to run +real alerting. Version disclosure on a health endpoint is a mild and widely accepted +exposure; it aids version fingerprinting but grants no access, leaks no tenant data, and +reveals nothing an attacker could not infer from behavior. + +This spec introduces no new permission concept and changes no existing one: + +- The public allowlist is **not** modified. `/health` was already public. +- `/api/v1/system/info` and `/api/v1/system/health` remain authenticated. AC-14 pins this. +- No per-user or per-workspace scoping is involved; the value is process-global and + identical for every caller. + +## Failure Modes + +| Condition | Behavior | +|---|---| +| Built without ldflags | `version` is `"dev"` (the compiled-in default), never empty. AC-8. | +| ldflag injected as empty string | `setBuildInfo` (`main.go:251`) skips empty values, so the `"dev"` default is retained. AC-11. | +| Request arrives before readiness | 503 with `status: "starting"` plus `version`. AC-5, AC-6, AC-7. | +| Request arrives during shutdown | Unchanged from today; this spec adds no new shutdown behavior. | +| Consumer strictly parses the JSON body | See the compatibility audit below. No in-tree consumer breaks. | + +The handler performs no I/O, no database access, and no locking beyond the existing +`atomic.Bool` read. Reading an already-populated string field cannot fail, so this change +introduces no new error path and cannot make `/health` slower, flaky, or dependent on +another subsystem. That property matters: `/health` gates container start, so it must +never acquire a new dependency. + +## Consumer Compatibility Audit + +Every in-tree `/health` consumer was checked against an additive body field. + +| Consumer | File | How it reads `/health` | Impact | +|---|---|---|---| +| K8s liveness + readiness | `k8s/deployment.yaml:37-47` | HTTP status only | None | +| Go launcher | `internal/launcher/health.go:44-80` | Status code; body drained and discarded | None | +| CLI | `apps/cli/src/health.ts` | `res.ok` only | None | +| Desktop shell (Tauri) | `apps/desktop/src-tauri/src/backend.rs:541-567` | Raw socket; parses status line + `x-kandev-desktop-health-token` header. Its own test body is literally `"ignored"` (`backend.rs:1032`) | None | +| Desktop smoke test | `apps/desktop/e2e/desktop-launch-smoke.mjs` | URL match on a fake runtime | None | +| Playwright fixture | `apps/web/e2e/fixtures/backend.ts:461` | Waits for HTTP readiness | None | +| Homebrew service | `scripts/release/kandev.rb` | Polls the URL | None | +| Docker docs example | `docs/docker.md` | `curl -f`, status only | None | +| Public operations doc | `docs/public/operations.md:70` | **Pins the exact JSON body** | **Must be updated — AC-15** | + +No consumer deserializes the body into a fixed struct that would reject an unknown field, +so the change is backward compatible for every known caller. + +## Testing Plan + +| Layer | What | Count | +|---|---|---| +| Unit (Go) | Ready path: 200, `version` present, correct value, exact key set (AC-1..4) | +1 | +| Unit (Go) | Starting path: 503, `version` present and correct, exact key set (AC-5..7) | +1 | +| Unit (Go) | Default `"dev"` when unstamped; injected value when stamped; never empty (AC-8,9,11) | +1 | +| Unit (Go) | `/health` and `/api/v1/system/info` agree on the value (AC-10) | +1 | +| Unit (Go) | Auth enabled: `/health` unauthenticated returns body with `version`; `/system/info` still rejected (AC-12,14) | +1 | + +These are the first tests to assert the `/health` body at all, so they also lock in +`status`, `service`, and `mode` against future accidental drift (AC-3, AC-13). + +The exact-key-set assertions (AC-4, AC-7) are deliberate: they turn any future silent +addition to this public payload into a failing test, which is the behavior a frozen public +contract wants. + +No new E2E test is required. The existing Playwright fixture already exercises `/health` +on every run and would fail if the endpoint regressed. + +## Out of Scope + +- `GET /api/v1/system/health` (the diagnostics/issues endpoint). Untouched. +- `GET /api/v1/system/info`. Untouched; it remains the detailed build-info surface and + keeps its `commit`, `build_time`, `go_version`, `os`, `arch`, `boot_id`, `started_at` + fields. +- The WS `health.check` action (`internal/gateway/websocket/handler.go:74`), which returns + a similar `{status, service, mode}` shape. Adding the version there is a reasonable + follow-up but is not required by the monitoring/alerting goal and is not specced here. +- Adding `commit`, `build_time`, uptime, or any other build metadata to `/health`. The + endpoint stays minimal; `/system/info` is the place for detail. +- The agentctl server's own `/health` (`internal/agentctl/server/api/server.go:90`) and + the office test harness `/health` (`internal/office/testharness/routes.go:94`). Separate + servers, separate contracts. +- Renaming, moving, or changing the auth posture of any endpoint. +- Changing the `kandev_meta.kandev_version` database key or the share-snapshot + `kandev_version` field. Same words, unrelated contracts. + +## Decisions + +Resolved with the requester on 2026-08-07, before this spec was frozen. + +1. **Field name: `version`** (not `kandev_version`, not both). It matches + `/api/v1/system/info` exactly, so the two endpoints agree on one name for one value. + The payload already carries `"service":"kandev"`, so `version` unambiguously reads as + "version of the service named kandev"; `kandev_version` beside `service: kandev` would + be redundant. AC-10 pins the agreement. +2. **Present in both 200 and 503.** A backend stuck in startup is exactly when an operator + most needs to know which build is stuck — it distinguishes a bad rollout from a slow + disk. AC-6 pins this. +3. **Always exposed, never gated.** No auth condition, no runtime feature flag. Gating + would defeat the stated monitoring goal, and a runtime flag would add a registry entry, + profile defaults, and a disabled-path test for a single string field. AC-12 pins this. + +## Documentation Impact + +- `docs/public/operations.md:70` pins the exact `/health` body and MUST be updated + (AC-15). It is the only doc that reproduces the payload verbatim. +- `docs/docker.md`, `docs/k8s.md`, `docs/run-as-a-service.md`, `docs/public/cli.md`, and + `docs/public/authentication.md` reference `/health` but do not reproduce its body, so + they need no change. +- Consider noting in `docs/public/operations.md` that `/health` is now the + credential-free way to read the running version, since that is the operator-visible + benefit. + +## Rollback + +Revert the handler change and the doc change. No migration, no persisted state, no config +key, no feature flag, and no cross-service contract is involved, so a straight revert is +complete and instant. A consumer that starts depending on the new field would see it +disappear, which is the ordinary cost of reverting an additive API field. + +## Files Reference + +| File | Change | +|---|---| +| `apps/backend/internal/backendapp/helpers.go:675-684` | Add `version: p.version` to both the 503 and 200 JSON bodies | +| `apps/backend/internal/backendapp/*_test.go` | New tests for the `/health` body (none exist today) | +| `docs/public/operations.md:70` | Update the documented example payload | + +## Related + +- ADR `docs/decisions/0008-db-upgrade-safety.md` — the `kandev_meta.kandev_version` key, + a different contract that shares the name. +- `docs/public/authentication.md` — documents the unauthenticated allowlist that `/health` + belongs to. + +## Implementation Notes + +Recorded 2026-08-07, during the build against this frozen spec. + +- **`healthHandler` extraction (documented assumption).** The spec's Files Reference + called for editing `helpers.go:675-684` in place. Implementing it that way would have + left the handler as an inline closure with no way to unit-test AC-1..11 without + standing up the full `registerRoutes` dependency graph (auth service, task service, + gateway, lifecycle manager, etc. — far outside what a handler-body change warrants). + Extracted the closure to a named `healthHandler(p routeParams) gin.HandlerFunc` in the + same file, immediately below `registerRoutes`, with `p.router.GET("/health", ...)` + unchanged at the call site. Behavior, route path, and JSON shape are identical; this is + a pure testability refactor, not a contract change. +- **AC-10 test strategy.** Rather than an HTTP integration test hitting both live routes, + the test constructs `healthHandler` and `info.Handler`/`info.NewService` with the same + input string and asserts the JSON bodies agree. This is sufficient because both + handlers are proven (by reading `main.go:801,1830`) to be fed the same package-level + `Version` var, sampled after `setBuildInfo` runs in both cases — the test pins that + invariant at the handler layer rather than re-deriving it via a full boot. +- **AC-14 test placement.** Added a `system info` case (`blocked: true`) to the existing + `TestEnabledModeAllowlistMatrix` table in `internal/auth/httpmw/middleware_test.go` + rather than standing up the real `/api/v1/system/info` route (which needs a DB pool, + event bus, and the full `systemsvc.Provide` graph) — that test already exists + specifically to pin the allowlist and is the natural home for a new blocked-path + assertion. +- **Environment: pre-existing, unrelated test/lint failures observed in this sandbox.** + Confirmed via `git stash` (reproduces identically on unmodified branch HEAD, so not + caused by this change): + - `go test ./...`: `internal/agentctl/server/config` (`TestCollectAgentEnvGitHubCLIShimSurvivesLoginShell`, + depends on this machine's `gh` CLI/login-shell PATH), `internal/gitlab` + (`TestPollerStartRecordsHealthForEveryConfiguredWorkspace`, + `TestEnvironmentTokenAllowsImmutableStartupHost`, + `TestResolveGitLabExecutionCredentialsByAuthMethod`, host-matching config depends on + local env), `internal/repoclone` + (`TestEnsureWorkspaceClonedWithBasicAuthKeepsCredentialScopedToGitChild`, passes in + isolation — resource contention under full-suite parallelism), and intermittently + `internal/agentctl/server/process` and `internal/agent/runtime/routingerr` + (subprocess/timing-sensitive). None touch `backendapp`, `auth/httpmw`, or + `system/info`; `go test ./internal/backendapp/... ./internal/auth/httpmw/... + ./internal/system/info/...` is green. + - `make lint` → `lint-harness`: `.github/scripts/lint-harness-files.py` uses + `str | None` type syntax requiring Python 3.10+; this sandbox has Python 3.9.6. + `lint-backend` (golangci-lint, 0 issues), `lint-web` (eslint, 0 warnings), and + `lint-architecture` all pass when run directly. + +## Testing Notes (QA pass, 2026-08-08) + +- **Mutation testing, not just green tests.** Each new assertion was verified to fail + when the behavior it guards is broken, by temporarily reverting `healthHandler` (and, + for AC-14, `isPublicPath`) and confirming the specific test fails, then restoring: + dropping `version` from the JSON body fails AC-1..4/AC-12; adding an undeclared field + fails the AC-4 exact-key-set assertion; widening the allowlist to include + `/api/v1/system/info` fails the added `system info` case in + `TestEnabledModeAllowlistMatrix`; moving the desktop-token header write outside the + ready-only branch fails the new header test below. All four mutations were confirmed + to fail their respective test, then the code was restored and confirmed green again. +- **Added `TestHealthHandlerDesktopTokenHeaderOnlyOnReadyPath`.** The spec's "## What" + section states the desktop health-token header "SHALL continue to be set on the 200 + path only, unchanged," but this wasn't captured by a numbered AC and had no direct + test before this change extracted the handler into `healthHandler` — a refactor that + touches the exact code region setting that header. Added a test asserting the header + is absent on the 503 path and present with the correct value on the 200 path, and + mutation-tested it (moved the header write above the `ready` check — test caught it). +- **Independently re-verified the Consumer Compatibility Audit table**, not just trusted + it: read `internal/launcher/health.go` (`probeHealth` discards the body via + `io.Copy(io.Discard, ...)`), `apps/cli/src/health.ts` (checks `res.ok` only, no body + parsing), and `apps/desktop/src-tauri/src/backend.rs` (`read_http_response_head` stops + at the `\r\n\r\n` header terminator and never reads the body at all). All three match + the audit table's claims. Also independently searched `docs/**` for any other file + reproducing the `/health` JSON body verbatim (beyond `docs/public/operations.md`, + already updated) — found only `docs/WEBSOCKET_API.md`'s documentation of the separate + WS `health.check` action (explicitly Out of Scope in this spec) and a few unrelated + `"status":"ok"` mentions in other features' docs/specs. No additional doc needs + updating. +- **Live end-to-end verification** (real compiled binaries, not just unit tests): + confirmed unstamped build reports `"version":"dev"` on `/health`; a build stamped via + `-ldflags "-X main.Version=9.9.9-qa-test ..."` reports that value identically on both + `/health` and `/api/v1/system/info` (AC-9, AC-10 live); with `KANDEV_FEATURES_AUTH=true` + and no credential, `/health` returns 200 with `version` while + `/api/v1/system/info` returns 401 (AC-12, AC-14 live). +- **Safe-testing rule for live-binary QA in a Kandev-hosted session:** a + `KANDEV_HOME_DIR` override alone does not isolate a throwaway QA binary from + the real `~/.kandev` install. This session's own ambient environment (it runs + as a Kandev-orchestrated task) exports `KANDEV_DATABASE_PATH`, which viper's + `AutomaticEnv()` binds independently of `KANDEV_HOME_DIR` — a QA binary + started this way inherits it and boots against the live database instead of + the intended scratch one. Pass an explicit `KANDEV_DATABASE_PATH` override + alongside `KANDEV_HOME_DIR`, or unset inherited `KANDEV_*` vars first, before + running a live compiled binary for QA in this kind of environment. Not a + defect in the code under test.