From a9cb65d944ce91b8cd168320f7b22695e83ead9e Mon Sep 17 00:00:00 2001 From: Clovis Muneza Date: Wed, 15 Apr 2026 02:52:11 -0400 Subject: [PATCH 1/8] Add design spec for services.LookupAny MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the silent-failure gaps left over from the rustfs and mailpit binary-service migrations. Six callsites currently consult only the Docker registry via services.Lookup and silently mishandle binary services (s3, mail) — the wizard hides them, post-wizard provisioning skips them, doctor reports them as broken, install validation rejects them, and `pv service:env` errors on them. Spec introduces a single new helper services.LookupAny returning a tagged-union (Kind, BinaryService, Service, error), then walks each of the six callsites with a small kind-branched fix. Out of scope: the dispatcher's resolveKind (different invariant), stop.go's applyFallbacks issue (different bug shape), and the ReadyCheck sum-type tightening (separate type-design PR). --- .../2026-04-15-services-lookupany-design.md | 424 ++++++++++++++++++ 1 file changed, 424 insertions(+) create mode 100644 docs/superpowers/specs/2026-04-15-services-lookupany-design.md diff --git a/docs/superpowers/specs/2026-04-15-services-lookupany-design.md b/docs/superpowers/specs/2026-04-15-services-lookupany-design.md new file mode 100644 index 0000000..6392751 --- /dev/null +++ b/docs/superpowers/specs/2026-04-15-services-lookupany-design.md @@ -0,0 +1,424 @@ +# `services.LookupAny` — Unified Service Lookup Across Docker + Binary Registries + +**Date:** 2026-04-15 +**Status:** Approved + +## Relationship to prior specs + +This spec follows the rustfs and mailpit native-binary migrations. Those PRs introduced a second registry — `binaryRegistry` — alongside the existing Docker `registry` in `internal/services/`. The PR review for the mailpit migration identified six callsites that consult only the Docker registry via `services.Lookup` and silently mishandle any service that lives in `binaryRegistry`. This spec defines the helper and per-callsite fixes that close those gaps. + +This spec is **purely additive plus targeted callsite fixes**. No interface changes to `Service` or `BinaryService`. No changes to the supervisor, the dispatcher, or the registry. + +## Problem + +After the rustfs (s3) and mailpit (mail) binary-service migrations, six callsites in the codebase still call `services.Lookup(name)` and treat a "name not in the Docker registry" error as "service does not exist." For names that live in `binaryRegistry` (currently `s3` and `mail`), this produces user-visible silent failures: + +| Callsite | User-visible symptom | +|---|---| +| `cmd/install.go:61` | `pv install --with=service[mail]` errors out: "unknown service" | +| `cmd/setup.go:70-74` | Setup wizard's service multi-select hides mail and s3 entirely | +| `cmd/setup.go:252-256` | Even if a binary service is somehow selected, the post-wizard provisioning loop silently skips it | +| `cmd/doctor.go:621` | `pv doctor` reports a healthy mail or s3 service as "unknown service type — registry may be out of date" | +| `internal/commands/service/env.go:45` | `pv service:env` (no args) prints "Skipping unknown service \"mail\"" for each binary service | +| `internal/commands/service/env.go:71` | `pv service:env mail` returns a hard error | + +These are all the same shape: a callsite that needs to read service metadata or behavior, fails open, and either omits the binary service or misreports it. None of them are in the `service:*` command path, which already routes through `internal/commands/service/dispatch.go`'s private `resolveKind` and handles binary services correctly. + +## Goals + +- Eliminate the six silent failures above so that any registered binary service is treated as a first-class citizen by `install`, `setup`, `doctor`, and `service:env`. +- Introduce a single small public helper (`services.LookupAny`) that consults both registries, so future callsites have an obvious path that doesn't replicate the bug. +- Keep the existing `services.Lookup` and `services.LookupBinary` functions untouched — they have callers (notably the `service:*` dispatcher and the `caddy` route generator) that intentionally consult one registry only. + +## Non-Goals + +- Do **not** refactor `internal/commands/service/dispatch.go`'s private `resolveKind` to delegate to `LookupAny`. `resolveKind` enforces an additional invariant (rejecting an attempt to register a binary service when a Docker-shaped registry entry of the same name already exists) that is irrelevant to the read-only callsites this spec targets. Collapsing them would weaken the dispatcher's contract. +- Do **not** address `internal/commands/service/stop.go:65-68`'s `applyFallbacksToLinkedProjects` issue. That callsite has a different bug shape (it iterates registered services and rewrites linked projects' `.env` files, which is wrong for binary services that are still supervised). It needs a kind-aware skip, not a `LookupAny` call. Tracked as a separate follow-up PR. +- Do **not** add a `String()` method to `Kind`. YAGNI; nothing currently formats `Kind` for display. +- Do **not** add E2E coverage of `pv setup` selecting mail in the wizard. Existing E2E `install.sh` exercises `pv setup` end-to-end without selecting binary services; expanding the fixtures is a separate effort. + +## Verified facts + +These were confirmed by reading the current code on `main` (commit `4a432f8`): + +- `services.Lookup` consults only `registry`; `services.LookupBinary` consults only `binaryRegistry`. Both already exist and behave as documented. +- `services.Available()` returns the deduplicated union of names from both registries (added in the mailpit migration). +- `internal/commands/service/dispatch.go:24-25` already does `LookupBinary` first, then `Lookup` — the lookup-order convention this spec adopts. +- The error string returned by `services.Lookup` is `unknown service %q (available: %s)` formatted via `services.Available()`. Adopting the same string in `LookupAny` means callsites switching from `Lookup` to `LookupAny` produce identical error UX. +- Both `Service` and `BinaryService` interfaces have `Name() string` and `DisplayName() string` methods. They diverge on everything else: `Service.EnvVars(projectName, port)` vs `BinaryService.EnvVars(projectName)`; `Service.DefaultVersion()` exists while `BinaryService` has no version concept (always "latest"). +- `service.RunAdd([]string{name})` (single-arg form) routes through `dispatch.go`'s `resolveKind` and correctly handles either kind. The post-wizard fix in `setup.go` can rely on this rather than calling `DefaultVersion()` first. **VERIFY during implementation** by reading `service.RunAdd`'s arg handling — fall back to passing `[name, "latest"]` if the single-arg form doesn't exist. + +## Architecture + +### New file + +| Path | Purpose | +|------|---------| +| `internal/services/lookup.go` | `Kind` enum + `LookupAny` function (the entire public API for this PR) | +| `internal/services/lookup_test.go` | 4 unit tests pinning the function's contract | + +### Modified files + +| Path | Change | +|------|--------| +| `cmd/install.go` | Replace `Lookup` validation in `parseWith` with `LookupAny` | +| `cmd/install_test.go` | Add 3 cases covering `--with=service[s3]`, `--with=service[mail]`, `--with=service[mongodb]` | +| `cmd/setup.go` | Extract `buildServiceOptions()` helper using `LookupAny`; route binary kind in post-wizard loop | +| `cmd/setup_test.go` | Test `buildServiceOptions()` (new file if it doesn't already exist) | +| `cmd/doctor.go` | Use `LookupAny` to skip binary services in the per-service Docker check loop, with an explanatory comment | +| `internal/commands/service/env.go` | Branch on `kind` in both the loop and single-service paths; call the right `EnvVars` signature | +| `internal/commands/service/env_test.go` | New file: 3 cases covering Docker-only, binary-only, and mixed registry states | + +No other files change. No interface methods are added or removed. + +## Components + +### `services.LookupAny` (`internal/services/lookup.go`) + +```go +package services + +import ( + "fmt" + "strings" +) + +type Kind int + +const ( + KindUnknown Kind = iota + KindDocker + KindBinary +) + +// LookupAny resolves a service name across both the Docker and binary +// registries. +// +// Lookup order is binary first, then Docker — matching the convention used +// by the service:* command dispatcher (internal/commands/service/dispatch.go). +// A name found in only one registry returns that kind. A name in neither +// registry returns KindUnknown plus a non-nil error whose text matches the +// Lookup error format so callsites switching from Lookup retain the same +// error UX. +// +// When kind != KindUnknown, exactly one of binSvc / docSvc is non-nil. +// +// For service:* commands that also need to enforce a no-collision invariant +// against an active registry.Registry, see resolveKind in +// internal/commands/service/dispatch.go. +func LookupAny(name string) (kind Kind, binSvc BinaryService, docSvc Service, err error) { + if svc, ok := LookupBinary(name); ok { + return KindBinary, svc, nil, nil + } + if svc, lookupErr := Lookup(name); lookupErr == nil { + return KindDocker, nil, svc, nil + } + return KindUnknown, nil, nil, fmt.Errorf( + "unknown service %q (available: %s)", + name, + strings.Join(Available(), ", "), + ) +} +``` + +### `cmd/install.go` change + +Before: +```go +if _, err := services.Lookup(s.name); err != nil { + return spec, fmt.Errorf("unknown service %q in --with (available: %s)", + s.name, strings.Join(services.Available(), ", ")) +} +``` + +After: +```go +if k, _, _, _ := services.LookupAny(s.name); k == services.KindUnknown { + return spec, fmt.Errorf("unknown service %q in --with (available: %s)", + s.name, strings.Join(services.Available(), ", ")) +} +``` + +Note: keep the existing `unknown service %q in --with` wrapper because it adds the `--with` context that's useful in the install command. Discard the inner error from `LookupAny` since its text would be redundant once wrapped. + +### `cmd/setup.go` changes + +Two changes in this file. + +**Change 1: Extract `buildServiceOptions()` helper.** + +Before (inline at line 69-75): +```go +var svcOpts []selectOption +for _, name := range services.Available() { + svc, _ := services.Lookup(name) + if svc != nil { + svcOpts = append(svcOpts, selectOption{label: svc.DisplayName(), value: name}) + } +} +``` + +After (inline call): +```go +svcOpts := buildServiceOptions() +``` + +New helper at the bottom of the file (or in a dedicated `setup_options.go` if `setup.go` is already large): + +```go +// buildServiceOptions returns the wizard's service multi-select options. +// Both Docker and binary services are listed using their DisplayName. +func buildServiceOptions() []selectOption { + names := services.Available() + out := make([]selectOption, 0, len(names)) + for _, name := range names { + kind, binSvc, docSvc, err := services.LookupAny(name) + if err != nil { + // Available() is the union of both registries; LookupAny over + // the same names should never miss. If it does, skip silently + // rather than error the wizard. + continue + } + var label string + switch kind { + case services.KindBinary: + label = binSvc.DisplayName() + case services.KindDocker: + label = docSvc.DisplayName() + } + out = append(out, selectOption{label: label, value: name}) + } + return out +} +``` + +**Change 2: Route binary kind in post-wizard provisioning.** + +Before (line 251-262): +```go +for _, name := range selectedServices { + svc, _ := services.Lookup(name) + if svc == nil { + continue + } + svcArgs := []string{name, svc.DefaultVersion()} + if err := service.RunAdd(svcArgs); err != nil { + if !errors.Is(err, ui.ErrAlreadyPrinted) { + ui.Fail(fmt.Sprintf("Service %s failed: %v", name, err)) + } + } +} +``` + +After: +```go +for _, name := range selectedServices { + kind, _, docSvc, err := services.LookupAny(name) + if err != nil { + ui.Fail(fmt.Sprintf("Service %s: %v", name, err)) + continue + } + var svcArgs []string + switch kind { + case services.KindBinary: + // Binary services are always "latest"; service:add ignores any + // explicit version for binary kinds. + svcArgs = []string{name} + case services.KindDocker: + svcArgs = []string{name, docSvc.DefaultVersion()} + } + if err := service.RunAdd(svcArgs); err != nil { + if !errors.Is(err, ui.ErrAlreadyPrinted) { + ui.Fail(fmt.Sprintf("Service %s failed: %v", name, err)) + } + } +} +``` + +If `service.RunAdd` requires a two-element slice for both kinds, pass `[]string{name, "latest"}` for the binary case instead. Confirm during implementation. + +### `cmd/doctor.go` change + +Before (line 616-633): +```go +for key, svc := range svcs { + svcName, version := services.ParseServiceKey(key) + + status := "unknown" + if engine != nil { + svcDef, lookupErr := services.Lookup(svcName) + if lookupErr != nil { + status = "lookup_error" + } else { + running, runErr := engine.IsRunning(...) + ... + } + } + ... +} +``` + +After: +```go +for key, svc := range svcs { + svcName, version := services.ParseServiceKey(key) + + // Skip binary services: they have no Docker container to probe. + // Binary supervision health is reported via `pv service:list` and + // `pv service:status`, which read the daemon's status snapshot + // directly. Including them here would couple doctor to the daemon's + // internal status format. + if kind, _, _, _ := services.LookupAny(svcName); kind == services.KindBinary { + continue + } + + status := "unknown" + if engine != nil { + svcDef, lookupErr := services.Lookup(svcName) + if lookupErr != nil { + status = "lookup_error" + } else { + ... + } + } + ... +} +``` + +The `lookup_error` branch becomes effectively unreachable for currently-known service names, but keep it as a guard against the (impossible-today) case of a registry entry whose name was never registered in either map. It now correctly indicates "registry is genuinely corrupt," matching the existing error text. + +### `internal/commands/service/env.go` changes + +Both call sites get the same kind-branch shape. + +Before (loop at line 43-52): +```go +for key, instance := range svcs { + svcName, _ := services.ParseServiceKey(key) + svc, err := services.Lookup(svcName) + if err != nil { + ui.Subtle(fmt.Sprintf("Skipping unknown service %q", svcName)) + continue + } + envVars := svc.EnvVars(projectName, instance.Port) + printEnvVars(key, envVars) +} +``` + +After: +```go +for key, instance := range svcs { + svcName, _ := services.ParseServiceKey(key) + kind, binSvc, docSvc, err := services.LookupAny(svcName) + if err != nil { + ui.Subtle(fmt.Sprintf("Skipping unknown service %q", svcName)) + continue + } + var envVars map[string]string + switch kind { + case services.KindBinary: + envVars = binSvc.EnvVars(projectName) + case services.KindDocker: + envVars = docSvc.EnvVars(projectName, instance.Port) + } + printEnvVars(key, envVars) +} +``` + +Single-service path (line 70-78) gets the same branch. No change to `printEnvVars`. + +## Data Flow + +``` +┌──────────────────┐ ┌─────────────────────┐ +│ any callsite │ ───▶ │ services.LookupAny │ +└──────────────────┘ └──────────┬──────────┘ + │ + ┌──────────┴──────────┐ + │ binaryRegistry? │ yes ──▶ KindBinary, *binSvc, nil, nil + └──────────┬──────────┘ + │ no + ┌──────────┴──────────┐ + │ registry? │ yes ──▶ KindDocker, nil, *docSvc, nil + └──────────┬──────────┘ + │ no + ▼ + KindUnknown, nil, nil, err +``` + +No I/O, no state, no goroutine concerns. Two map reads and an `fmt.Errorf` on the failure path. + +## Error Handling + +| Failure | Where caught | Behavior | +|---|---|---| +| Name not in either registry | `LookupAny` | Returns `KindUnknown` + error matching the existing `Lookup` text. Callers either propagate (with their own context wrapping) or print `ui.Subtle` and continue, preserving today's UX. | +| `Available()` lists a name that `LookupAny` then fails to find | Theoretically impossible; both consult the same maps. If it happens (e.g. concurrent registry mutation, which the codebase doesn't currently do), the wizard skips silently and continues — same fail-soft posture as the existing inline loop. | +| Caller forgets to check `kind` and dereferences a nil `binSvc` or `docSvc` | Caller bug — would panic. Mitigated by always pairing `LookupAny` with a `switch kind` block in the spec's example code. Tests cover the convention. | + +No new failure modes are introduced. The bugs being fixed all manifest as "silent skip" or "wrong error text"; after this PR, the same code paths produce correct output. + +## Testing Strategy + +### Unit tests + +**`internal/services/lookup_test.go`** — 4 cases: + +- `TestLookupAny_BinaryService` — `LookupAny("mail")` returns `(KindBinary, *Mailpit, nil, nil)`. +- `TestLookupAny_DockerService` — `LookupAny("mysql")` returns `(KindDocker, nil, *MySQL, nil)`. +- `TestLookupAny_Unknown` — `LookupAny("mongodb")` returns `(KindUnknown, nil, nil, err)`; assert error text contains `unknown service "mongodb"` and `available:`. +- `TestLookupAny_BinaryWinsOnCollision` — temporarily seed both registries with the same key (use `t.Cleanup` to restore), assert binary wins. + +**`cmd/install_test.go`** — extend with 3 cases: + +- `parseWith("service[s3]")` succeeds. +- `parseWith("service[mail]")` succeeds. +- `parseWith("service[mongodb]")` errors with text containing `unknown service`. + +**`cmd/setup_test.go`** — new tests for `buildServiceOptions()`: + +- All known service names appear in the returned slice with their `DisplayName()` as the label. +- Both binary services (`s3`, `mail`) and Docker services (`mysql`, `postgres`, `redis`) are present. + +**`cmd/doctor_test.go` (or wherever doctor tests live)** — 1 test: + +- After registering a binary service in a temp registry, the doctor check output does not contain a "lookup_error" or "registry may be out of date" entry for it. + +If `cmd/doctor.go` does not currently have a tested entry-point, defer this test to the integration / E2E level rather than introduce one ad hoc. + +**`internal/commands/service/env_test.go`** — new file, 3 cases: + +- All-services path with a registry containing only Docker services prints all of them. +- All-services path with a registry containing only binary services prints all of them. +- Single-service path: `pv service:env mail` returns the correct `MAIL_*` map. + +### Integration tests + +None required. The fixed callsites are not currently covered by integration tests at the daemon level; the unit tests cover the behavior changes. + +### E2E + +No new E2E phase. The existing `install.sh` exercises `pv setup` and would catch a regression that breaks the wizard for Docker services. Adding a fixture that selects mail in the wizard is a separate effort. + +### Explicitly NOT tested + +- Multi-version binary services (none exist; YAGNI). +- Concurrent registration into either registry from multiple `init()`s (would be a programmer error, not a runtime input). + +## Verification Items (before implementation starts) + +1. Confirm `service.RunAdd([]string{name})` (single-arg form) routes through `resolveKind` correctly for binary services. If only the two-arg form is supported, pass `[]string{name, "latest"}` for binary in `setup.go`'s post-wizard loop. + +Confirmed by inspection on 2026-04-15: + +- `cmd/setup_test.go` does not exist — create it. +- `internal/commands/service/env_test.go` does not exist — create it. +- `cmd/doctor_test.go` exists with a `newDoctorCmd()` scaffold and a `TestDoctor_EmptyHome` pattern using `t.TempDir()` + `t.Setenv("HOME", ...)`. Reuse the scaffold; add the new test alongside the existing ones. + +## Deferred + +- **`internal/commands/service/stop.go:65-68` `applyFallbacksToLinkedProjects` issue.** Different bug shape (rewrites linked projects' `.env` files for binary services that are still supervised). Tracked as a separate follow-up PR. +- **`ReadyCheck` sum-type tightening** (rejecting invalid `{}` and `{TCPPort: x, HTTPEndpoint: y}` states at construction time). Type-design improvement noted in the mailpit PR review; touches both `RustFS` and `Mailpit`. Separate PR. +- **`TestBuildSupervisorProcess_Mailpit`** — closes the per-service supervisor wiring test gap noted in the mailpit PR review. Separate PR. +- **E2E coverage that selects mail in `setup.sh` wizard fixtures.** Separate effort. +- **Refactor of `internal/commands/service/dispatch.go`'s `resolveKind` to delegate to `LookupAny`.** Considered; rejected because `resolveKind` enforces a no-collision invariant that `LookupAny` doesn't, and collapsing them would weaken the dispatcher's contract. From 74a93714aff1a215877681ba2523e8894563b375 Mon Sep 17 00:00:00 2001 From: Clovis Muneza Date: Wed, 15 Apr 2026 02:56:59 -0400 Subject: [PATCH 2/8] Add implementation plan for services.LookupAny Six tasks executing the design from docs/superpowers/specs/2026-04-15-services-lookupany-design.md: 1. Pre-flight verification of service.RunAdd's arg shape. 2. services.LookupAny + 4 unit tests (TDD). 3. cmd/setup.go: extract buildServiceOptions + post-wizard branch. 4. internal/commands/service/env.go: extract envVarsFor dispatch helper so both call sites and the test exercise the same code path. 5. cmd/doctor.go: skip binary services in per-service Docker check. 6. cmd/install.go: parseWith validation accepts binary services. Each task ends with a commit; six commits to land on the branch. --- .../plans/2026-04-15-services-lookupany.md | 913 ++++++++++++++++++ 1 file changed, 913 insertions(+) create mode 100644 docs/superpowers/plans/2026-04-15-services-lookupany.md diff --git a/docs/superpowers/plans/2026-04-15-services-lookupany.md b/docs/superpowers/plans/2026-04-15-services-lookupany.md new file mode 100644 index 0000000..83405eb --- /dev/null +++ b/docs/superpowers/plans/2026-04-15-services-lookupany.md @@ -0,0 +1,913 @@ +# `services.LookupAny` Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add `services.LookupAny` (a tagged-union helper that resolves a service name across both Docker and binary registries) and fix the six callsites that currently consult only the Docker registry and silently mishandle binary services (s3, mail). + +**Architecture:** Purely additive helper plus six small kind-branched callsite fixes. No interface changes, no supervisor changes. Mirrors the existing private `resolveKind` lookup-order convention (binary first, Docker second). + +**Tech Stack:** Go, existing `services` package, existing `cobra` command tree. + +**Spec:** `docs/superpowers/specs/2026-04-15-services-lookupany-design.md` + +**Branch:** `feat/services-lookupany` (already created). Each task ends with a commit; six commits total. + +--- + +## File Structure + +| Path | Action | Responsibility | +|------|--------|---------------| +| `internal/services/lookup.go` | Create | `Kind` enum + `LookupAny` function (the new public helper) | +| `internal/services/lookup_test.go` | Create | Unit tests pinning lookup-order, both-kinds, and unknown-name behavior | +| `cmd/install.go` | Modify | Replace `Lookup` validation in `parseWith` with `LookupAny` | +| `cmd/install_test.go` | Modify | Add 3 cases covering `--with=service[s3|mail|mongodb]` | +| `cmd/setup.go` | Modify | Extract `buildServiceOptions()` helper using `LookupAny`; route binary kind in post-wizard provisioning loop | +| `cmd/setup_test.go` | Create | Test `buildServiceOptions()` — verifies all binary + Docker services appear | +| `cmd/doctor.go` | Modify | Skip binary services in the per-service Docker check loop | +| `cmd/doctor_test.go` | Modify | Add a test verifying binary services don't produce "lookup_error" output | +| `internal/commands/service/env.go` | Modify | Branch on `kind` in both the all-services loop and the single-service path | +| `internal/commands/service/env_test.go` | Create | 3 cases: Docker-only registry, binary-only registry, single-service for both kinds | + +--- + +## Task 1: Pre-flight verification + +**Files:** +- None modified. Verification only. + +The spec lists one verification item that must be confirmed before coding starts. It has already been spot-checked by the plan author, but the implementer should re-confirm in case the codebase moved. + +- [ ] **Step 1: Confirm `service.RunAdd` accepts a single-arg slice for binary services** + +Read `internal/commands/service/add.go` lines 23-62. Confirm: +- `addCmd.Args` is `cobra.RangeArgs(1, 2)` (accepts 1 or 2 args). +- The `kindBinary` branch at line 51-52 calls `addBinary(cmd.Context(), reg, binSvc)` with no `args[1]` reference. +- The `kindDocker` branch at line 53-58 only consults `args[1]` if `len(args) > 1`. + +If those conditions hold, `service.RunAdd([]string{name})` (single-arg) is the correct call for binary services in `setup.go`'s post-wizard loop. Continue to Task 2. + +If `addCmd.Args` has changed or the binary branch now requires a version arg, pass `[]string{name, "latest"}` instead in Task 6 — adjust the implementation code accordingly. + +--- + +## Task 2: `services.LookupAny` + unit tests + +**Files:** +- Create: `internal/services/lookup.go` +- Create: `internal/services/lookup_test.go` + +Pure addition. TDD: tests first, fail, implement, pass, commit. + +- [ ] **Step 1: Write the failing tests** + +Create `internal/services/lookup_test.go`: + +```go +package services + +import ( + "strings" + "testing" +) + +func TestLookupAny_BinaryService(t *testing.T) { + kind, binSvc, docSvc, err := LookupAny("mail") + if err != nil { + t.Fatalf("LookupAny(\"mail\") error = %v", err) + } + if kind != KindBinary { + t.Errorf("kind = %v, want KindBinary", kind) + } + if binSvc == nil { + t.Error("binSvc is nil; want non-nil for binary kind") + } + if docSvc != nil { + t.Errorf("docSvc = %#v, want nil for binary kind", docSvc) + } +} + +func TestLookupAny_DockerService(t *testing.T) { + kind, binSvc, docSvc, err := LookupAny("mysql") + if err != nil { + t.Fatalf("LookupAny(\"mysql\") error = %v", err) + } + if kind != KindDocker { + t.Errorf("kind = %v, want KindDocker", kind) + } + if docSvc == nil { + t.Error("docSvc is nil; want non-nil for docker kind") + } + if binSvc != nil { + t.Errorf("binSvc = %#v, want nil for docker kind", binSvc) + } +} + +func TestLookupAny_Unknown(t *testing.T) { + kind, binSvc, docSvc, err := LookupAny("mongodb") + if err == nil { + t.Fatal("LookupAny(\"mongodb\") error = nil; want non-nil for unknown name") + } + if kind != KindUnknown { + t.Errorf("kind = %v, want KindUnknown", kind) + } + if binSvc != nil || docSvc != nil { + t.Errorf("binSvc=%v docSvc=%v; want both nil", binSvc, docSvc) + } + if !strings.Contains(err.Error(), `unknown service "mongodb"`) { + t.Errorf("error %q missing expected text", err) + } + if !strings.Contains(err.Error(), "available:") { + t.Errorf("error %q missing available list", err) + } +} + +func TestLookupAny_BinaryWinsOnCollision(t *testing.T) { + // Pin the lookup-order invariant by temporarily seeding both registries + // with the same key. Restore via t.Cleanup so other tests are unaffected. + const key = "collisiontest" + + // Stash any pre-existing entries (defensive — there should be none). + prevBin, hadBin := binaryRegistry[key] + prevDoc, hadDoc := registry[key] + t.Cleanup(func() { + if hadBin { + binaryRegistry[key] = prevBin + } else { + delete(binaryRegistry, key) + } + if hadDoc { + registry[key] = prevDoc + } else { + delete(registry, key) + } + }) + + binaryRegistry[key] = &Mailpit{} // any BinaryService will do + registry[key] = &MySQL{} // any Service will do + + kind, binSvc, docSvc, err := LookupAny(key) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if kind != KindBinary { + t.Errorf("kind = %v, want KindBinary (binary should win on collision)", kind) + } + if binSvc == nil { + t.Error("binSvc is nil; want non-nil") + } + if docSvc != nil { + t.Errorf("docSvc = %#v, want nil (binary won)", docSvc) + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +go test ./internal/services/ -run LookupAny -v +``` + +Expected: FAIL — `undefined: LookupAny`, `undefined: KindBinary`, `undefined: KindDocker`, `undefined: KindUnknown`. + +- [ ] **Step 3: Create the implementation** + +Create `internal/services/lookup.go`: + +```go +package services + +import ( + "fmt" + "strings" +) + +// Kind classifies which registry a service name resolves to. +type Kind int + +const ( + KindUnknown Kind = iota + KindDocker + KindBinary +) + +// LookupAny resolves a service name across both the Docker and binary +// registries. +// +// Lookup order is binary first, then Docker — matching the convention used +// by the service:* command dispatcher (internal/commands/service/dispatch.go). +// A name found in only one registry returns that kind. A name in neither +// registry returns KindUnknown plus a non-nil error whose text matches the +// Lookup error format so callsites switching from Lookup retain the same +// error UX. +// +// When kind != KindUnknown, exactly one of binSvc / docSvc is non-nil. +// +// For service:* commands that also need to enforce a no-collision invariant +// against an active registry.Registry, see resolveKind in +// internal/commands/service/dispatch.go. +func LookupAny(name string) (kind Kind, binSvc BinaryService, docSvc Service, err error) { + if svc, ok := LookupBinary(name); ok { + return KindBinary, svc, nil, nil + } + if svc, lookupErr := Lookup(name); lookupErr == nil { + return KindDocker, nil, svc, nil + } + return KindUnknown, nil, nil, fmt.Errorf( + "unknown service %q (available: %s)", + name, + strings.Join(Available(), ", "), + ) +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +```bash +gofmt -w internal/services/ +go vet ./internal/services/ +go test ./internal/services/ -v +``` + +Expected: PASS for all four `TestLookupAny_*` plus all pre-existing tests in the package. + +- [ ] **Step 5: Commit** + +```bash +git add internal/services/lookup.go internal/services/lookup_test.go +git commit -m "Add services.LookupAny helper + +Resolves a service name across both Docker and binary registries, +returning a tagged union (Kind, BinaryService, Service, error). Binary +wins on a hypothetical collision — matching the order used by the +service:* command dispatcher's private resolveKind helper. + +Closes the gap that left several callsites only consulting the Docker +registry and silently mishandling binary services (s3, mail). Per-callsite +fixes follow in subsequent commits." +``` + +--- + +## Task 3: Wire `cmd/setup.go` — extract `buildServiceOptions` + branch the post-wizard loop + +**Files:** +- Modify: `cmd/setup.go` +- Create: `cmd/setup_test.go` + +Two changes in `cmd/setup.go`: +1. Replace the inline service-options loop (lines 69-75) with a call to a new `buildServiceOptions()` helper. +2. Branch on `kind` in the post-wizard provisioning loop (lines 251-262). + +- [ ] **Step 1: Write the failing test for `buildServiceOptions`** + +Create `cmd/setup_test.go`: + +```go +package cmd + +import ( + "testing" +) + +func TestBuildServiceOptions_IncludesBothKinds(t *testing.T) { + opts := buildServiceOptions() + if len(opts) == 0 { + t.Fatal("buildServiceOptions() returned empty; want at least one option") + } + + // Pin specific names from each registry. mysql is Docker; mail and s3 are binary. + want := []string{"mysql", "postgres", "redis", "mail", "s3"} + for _, name := range want { + found := false + for _, opt := range opts { + if opt.value == name { + found = true + if opt.label == "" { + t.Errorf("option %q has empty label", name) + } + break + } + } + if !found { + t.Errorf("buildServiceOptions() missing %q", name) + } + } +} + +func TestBuildServiceOptions_LabelsUseDisplayName(t *testing.T) { + opts := buildServiceOptions() + for _, opt := range opts { + // DisplayName for mail should be "Mail (Mailpit)" — not "mail". + if opt.value == "mail" && opt.label != "Mail (Mailpit)" { + t.Errorf("mail label = %q, want %q", opt.label, "Mail (Mailpit)") + } + // DisplayName for s3 should be "S3 Storage (RustFS)". + if opt.value == "s3" && opt.label != "S3 Storage (RustFS)" { + t.Errorf("s3 label = %q, want %q", opt.label, "S3 Storage (RustFS)") + } + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +go test ./cmd/ -run TestBuildServiceOptions -v +``` + +Expected: FAIL — `undefined: buildServiceOptions`. + +- [ ] **Step 3: Extract the helper in `cmd/setup.go`** + +In `cmd/setup.go`, replace the inline loop at lines 68-75: + +Before: +```go + // Service options. + var svcOpts []selectOption + for _, name := range services.Available() { + svc, _ := services.Lookup(name) + if svc != nil { + svcOpts = append(svcOpts, selectOption{label: svc.DisplayName(), value: name}) + } + } +``` + +After: +```go + // Service options. + svcOpts := buildServiceOptions() +``` + +Add the helper function at the bottom of `cmd/setup.go` (place it after the `setupCmd` definition; if a natural insertion point is unclear, put it just above the closing of the file): + +```go +// buildServiceOptions returns the wizard's service multi-select options. +// Both Docker and binary services are listed using their DisplayName so that +// binary-only services (mail, s3) are visible in the picker. +func buildServiceOptions() []selectOption { + names := services.Available() + out := make([]selectOption, 0, len(names)) + for _, name := range names { + kind, binSvc, docSvc, err := services.LookupAny(name) + if err != nil { + // Available() is the union of both registries; LookupAny over + // the same names should never miss. If it does, skip silently + // rather than break the wizard. + continue + } + var label string + switch kind { + case services.KindBinary: + label = binSvc.DisplayName() + case services.KindDocker: + label = docSvc.DisplayName() + } + out = append(out, selectOption{label: label, value: name}) + } + return out +} +``` + +- [ ] **Step 4: Update the post-wizard provisioning loop** + +In `cmd/setup.go`, replace the loop at lines 249-263: + +Before: +```go + // Spin up selected services. + if len(selectedServices) > 0 { + fmt.Fprintln(os.Stderr) + for _, name := range selectedServices { + svc, _ := services.Lookup(name) + if svc == nil { + continue + } + svcArgs := []string{name, svc.DefaultVersion()} + if err := service.RunAdd(svcArgs); err != nil { + if !errors.Is(err, ui.ErrAlreadyPrinted) { + ui.Fail(fmt.Sprintf("Service %s failed: %v", name, err)) + } + } + } + } +``` + +After: +```go + // Spin up selected services. + if len(selectedServices) > 0 { + fmt.Fprintln(os.Stderr) + for _, name := range selectedServices { + kind, _, docSvc, lookupErr := services.LookupAny(name) + if lookupErr != nil { + ui.Fail(fmt.Sprintf("Service %s: %v", name, lookupErr)) + continue + } + var svcArgs []string + switch kind { + case services.KindBinary: + // Binary services are unversioned; service:add ignores any + // explicit version (verified in Task 1). + svcArgs = []string{name} + case services.KindDocker: + svcArgs = []string{name, docSvc.DefaultVersion()} + } + if err := service.RunAdd(svcArgs); err != nil { + if !errors.Is(err, ui.ErrAlreadyPrinted) { + ui.Fail(fmt.Sprintf("Service %s failed: %v", name, err)) + } + } + } + } +``` + +- [ ] **Step 5: Run tests to verify they pass** + +```bash +gofmt -w cmd/ +go vet ./cmd/ +go test ./cmd/ -run TestBuildServiceOptions -v +go test ./cmd/ -v +``` + +Expected: PASS for both `TestBuildServiceOptions_*` tests, plus all pre-existing `cmd/` tests. + +- [ ] **Step 6: Commit** + +```bash +git add cmd/setup.go cmd/setup_test.go +git commit -m "setup.go: list binary services in wizard and provision them + +Extracts buildServiceOptions() so the wizard's service multi-select +includes both Docker and binary services (mail, s3). The post-wizard +provisioning loop now branches on kind and calls service:add with the +appropriate arg form for each kind — the binary branch uses the +single-arg form which routes through resolveKind correctly. + +Fixes two of the six silent failures called out in the mailpit PR review." +``` + +--- + +## Task 4: Wire `internal/commands/service/env.go` — extract dispatch helper + branch on kind + +**Files:** +- Modify: `internal/commands/service/env.go` +- Create: `internal/commands/service/env_test.go` + +Both call sites in `env.go` need the same kind-branch shape (calling the right `EnvVars` signature for each kind). To avoid duplication and gain a real test surface, extract the dispatch into a small helper `envVarsFor` that **the production code itself uses** — so the tests pin the production behavior, not a parallel implementation. + +- [ ] **Step 1: Write the failing tests** + +Create `internal/commands/service/env_test.go`: + +```go +package service + +import ( + "strings" + "testing" +) + +func TestEnvVarsFor_BinaryService(t *testing.T) { + got, err := envVarsFor("mail", "anyproject", 0) + if err != nil { + t.Fatalf("envVarsFor(\"mail\") error = %v", err) + } + if got["MAIL_MAILER"] != "smtp" { + t.Errorf("MAIL_MAILER = %q, want smtp", got["MAIL_MAILER"]) + } + if got["MAIL_HOST"] != "127.0.0.1" { + t.Errorf("MAIL_HOST = %q, want 127.0.0.1", got["MAIL_HOST"]) + } +} + +func TestEnvVarsFor_DockerService(t *testing.T) { + // MySQL.EnvVars takes (projectName, port) and uses both. Pass a non-default + // port to verify the port arg is consulted — a regression that always + // passed 0 in the docker branch would silently set DB_PORT=0. + got, err := envVarsFor("mysql", "anyproject", 3306) + if err != nil { + t.Fatalf("envVarsFor(\"mysql\") error = %v", err) + } + if got["DB_PORT"] != "3306" { + t.Errorf("DB_PORT = %q, want 3306", got["DB_PORT"]) + } +} + +func TestEnvVarsFor_Unknown(t *testing.T) { + _, err := envVarsFor("mongodb", "anyproject", 0) + if err == nil { + t.Fatal("expected error for unknown service") + } + if !strings.Contains(err.Error(), `unknown service "mongodb"`) { + t.Errorf("error %q missing expected text", err) + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +go test ./internal/commands/service/ -run TestEnvVarsFor -v +``` + +Expected: FAIL — `undefined: envVarsFor`. + +- [ ] **Step 3: Add the `envVarsFor` helper in `env.go`** + +In `internal/commands/service/env.go`, add the helper function (place it just above `printEnvVars` at the bottom of the file): + +```go +// envVarsFor resolves a service name across both registries and returns the +// .env keys/values it injects into a linked project. Used by both the +// all-services and single-service code paths in envCmd so the per-kind +// dispatch lives in one place. +// +// Binary services have EnvVars(projectName) — port is fixed by the binary +// itself. Docker services have EnvVars(projectName, port) — port comes from +// the registry.ServiceInstance. +func envVarsFor(svcName, projectName string, port int) (map[string]string, error) { + kind, binSvc, docSvc, err := services.LookupAny(svcName) + if err != nil { + return nil, err + } + switch kind { + case services.KindBinary: + return binSvc.EnvVars(projectName), nil + case services.KindDocker: + return docSvc.EnvVars(projectName, port), nil + } + return nil, fmt.Errorf("unexpected kind %v for %q", kind, svcName) +} +``` + +- [ ] **Step 4: Update `env.go` — all-services loop** + +In `internal/commands/service/env.go`, replace lines 43-52: + +Before: +```go + fmt.Fprintln(os.Stderr) + for key, instance := range svcs { + svcName, _ := services.ParseServiceKey(key) + svc, err := services.Lookup(svcName) + if err != nil { + ui.Subtle(fmt.Sprintf("Skipping unknown service %q", svcName)) + continue + } + envVars := svc.EnvVars(projectName, instance.Port) + printEnvVars(key, envVars) + } +``` + +After: +```go + fmt.Fprintln(os.Stderr) + for key, instance := range svcs { + svcName, _ := services.ParseServiceKey(key) + envVars, err := envVarsFor(svcName, projectName, instance.Port) + if err != nil { + ui.Subtle(fmt.Sprintf("Skipping unknown service %q", svcName)) + continue + } + printEnvVars(key, envVars) + } +``` + +- [ ] **Step 5: Update `env.go` — single-service path** + +In `internal/commands/service/env.go`, replace lines 70-78: + +Before: +```go + svcName, _ := services.ParseServiceKey(key) + svc, err := services.Lookup(svcName) + if err != nil { + return err + } + + envVars := svc.EnvVars(projectName, instance.Port) + fmt.Fprintln(os.Stderr) + printEnvVars(key, envVars) +``` + +After: +```go + svcName, _ := services.ParseServiceKey(key) + envVars, err := envVarsFor(svcName, projectName, instance.Port) + if err != nil { + return err + } + + fmt.Fprintln(os.Stderr) + printEnvVars(key, envVars) +``` + +- [ ] **Step 6: Run tests + build** + +```bash +gofmt -w internal/commands/service/ +go vet ./internal/commands/service/ +go test ./internal/commands/service/ -v +go build ./... +``` + +Expected: PASS for `TestEnvVarsFor_*`, plus all existing `internal/commands/service/` tests. Build clean. + +- [ ] **Step 7: Commit** + +```bash +git add internal/commands/service/env.go internal/commands/service/env_test.go +git commit -m "service:env: print env vars for binary services correctly + +Extracts an envVarsFor helper that resolves the service name via +LookupAny and dispatches to the right EnvVars signature for each kind +(BinaryService.EnvVars(projectName) vs Service.EnvVars(projectName, +port)). Both call sites in envCmd now go through it, eliminating the +duplicated branching and giving us a real test surface. + +Previously \"pv service:env mail\" hard-errored and \"pv service:env\" +with no args printed \"Skipping unknown service\" for each registered +binary service. + +Fixes two of the six silent failures from the mailpit PR review." +``` + +--- + +## Task 5: Wire `cmd/doctor.go` — skip binary services in the per-service Docker check loop + +**Files:** +- Modify: `cmd/doctor.go` +- Modify: `cmd/doctor_test.go` + +The doctor loop (around line 616) iterates every registered service and asks Docker if its container is running. For binary services there's no container; the existing `lookup_error` branch wrongly reports them as "unknown service type — registry may be out of date." + +Per the spec's design decision (option B): skip binary services entirely. They have first-class observability via `pv service:list` / `pv service:status` / `pv service:logs`. Doctor's job is cross-cutting infra health. + +- [ ] **Step 1: Write the failing test** + +Add to `cmd/doctor_test.go` (the file exists; append the new test function at the bottom): + +```go +// TestDoctor_SkipsBinaryServices verifies that registering a binary service +// does NOT add a "lookup_error" / "unknown service type" entry to doctor's +// output. Binary services have their own observability and are intentionally +// skipped by the doctor's Docker-container check. +func TestDoctor_SkipsBinaryServices(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + if err := config.EnsureDirs(); err != nil { + t.Fatalf("EnsureDirs() error = %v", err) + } + + // Register a binary service in the registry on disk. + reg, err := registry.Load() + if err != nil { + t.Fatalf("registry.Load() error = %v", err) + } + tru := true + reg.Services["mail"] = ®istry.ServiceInstance{ + Kind: "binary", + Port: 1025, + Enabled: &tru, + } + if err := reg.Save(); err != nil { + t.Fatalf("reg.Save() error = %v", err) + } + + // Capture stderr output from doctor. + r, w, pipeErr := os.Pipe() + if pipeErr != nil { + t.Fatalf("os.Pipe() error = %v", pipeErr) + } + prevStderr := os.Stderr + os.Stderr = w + + cmd := newDoctorCmd() + cmd.SetArgs([]string{"doctor"}) + _ = cmd.Execute() + + w.Close() + os.Stderr = prevStderr + + buf := make([]byte, 64*1024) + n, _ := r.Read(buf) + output := string(buf[:n]) + + if strings.Contains(output, "lookup_error") { + t.Errorf("doctor output should not contain \"lookup_error\" for binary service; got:\n%s", output) + } + if strings.Contains(output, "unknown service type") { + t.Errorf("doctor output should not contain \"unknown service type\" for binary service; got:\n%s", output) + } + if strings.Contains(output, "registry may be out of date") { + t.Errorf("doctor output should not contain \"registry may be out of date\" for binary service; got:\n%s", output) + } +} +``` + +If `cmd/doctor_test.go` does not already import `strings`, add it to the import block. The other imports (`os`, `path/filepath`, `testing`, `config`, `registry`, `cobra`) are already present from the existing tests. Confirm via `head -15 cmd/doctor_test.go`. + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +go test ./cmd/ -run TestDoctor_SkipsBinaryServices -v +``` + +Expected: FAIL — the doctor will produce a `lookup_error` or `unknown service type` entry for the registered `mail` binary service because the current code iterates every registered service through the Docker-check path. + +(Note: doctor's overall command may exit non-zero due to other infra checks failing in the test env. That's fine — the assertions only inspect output text, not exit code.) + +- [ ] **Step 3: Update `cmd/doctor.go`** + +Find the loop around line 616. Add the binary-skip block at the top of the loop body. + +Before: +```go + for key, svc := range svcs { + svcName, version := services.ParseServiceKey(key) + + status := "unknown" + if engine != nil { + svcDef, lookupErr := services.Lookup(svcName) + if lookupErr != nil { + status = "lookup_error" + } else { +``` + +After: +```go + for key, svc := range svcs { + svcName, version := services.ParseServiceKey(key) + + // Skip binary services: they have no Docker container to probe. + // Binary supervision health is reported via `pv service:list` and + // `pv service:status`, which read the daemon's status snapshot + // directly. Including them here would couple doctor to the daemon's + // internal status format and produce misleading "lookup_error" + // output for healthy services (mail, s3). + if kind, _, _, _ := services.LookupAny(svcName); kind == services.KindBinary { + continue + } + + status := "unknown" + if engine != nil { + svcDef, lookupErr := services.Lookup(svcName) + if lookupErr != nil { + status = "lookup_error" + } else { +``` + +- [ ] **Step 4: Run tests + build** + +```bash +gofmt -w cmd/ +go vet ./cmd/ +go test ./cmd/ -run TestDoctor -v +go build ./... +``` + +Expected: PASS for `TestDoctor_SkipsBinaryServices` plus all pre-existing `TestDoctor_*` tests. + +- [ ] **Step 5: Commit** + +```bash +git add cmd/doctor.go cmd/doctor_test.go +git commit -m "doctor: skip binary services in per-service Docker check + +Doctor previously reported every registered binary service (mail, s3) +as \"unknown service type — registry may be out of date\" because the +loop fed every name through services.Lookup, which only consults the +Docker registry. Binary services have first-class observability via +pv service:list / pv service:status / pv service:logs; doctor's job +is cross-cutting infra health, not per-binary-service supervision +health. + +Fixes one of the six silent failures from the mailpit PR review." +``` + +--- + +## Task 6: Wire `cmd/install.go` — `--with=service[…]` validation accepts binary services + +**Files:** +- Modify: `cmd/install.go` +- Modify: `cmd/install_test.go` + +The smallest fix: replace one `services.Lookup` call in `parseWith` with `services.LookupAny`. + +- [ ] **Step 1: Write the failing tests** + +Append to `cmd/install_test.go` (after the existing `TestParseWith_*` tests; before `TestInstallCmd_AlreadyInstalled`): + +```go +func TestParseWith_BinaryServiceS3(t *testing.T) { + spec, err := parseWith("service[s3]") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(spec.services) != 1 { + t.Fatalf("expected 1 service, got %d", len(spec.services)) + } + if spec.services[0].name != "s3" { + t.Errorf("service[0].name = %q, want s3", spec.services[0].name) + } +} + +func TestParseWith_BinaryServiceMail(t *testing.T) { + spec, err := parseWith("service[mail]") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(spec.services) != 1 { + t.Fatalf("expected 1 service, got %d", len(spec.services)) + } + if spec.services[0].name != "mail" { + t.Errorf("service[0].name = %q, want mail", spec.services[0].name) + } +} + +func TestParseWith_UnknownServiceMongodb(t *testing.T) { + _, err := parseWith("service[mongodb]") + if err == nil { + t.Fatal("expected error for unknown service") + } + if !strings.Contains(err.Error(), `unknown service "mongodb"`) { + t.Errorf("error %q missing expected text", err) + } +} +``` + +If `cmd/install_test.go` does not already import `strings`, add it to the import block (the existing test file uses `os`, `path/filepath`, `testing`, `cobra` — `strings` is new). Confirm via `head -10 cmd/install_test.go`. + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +go test ./cmd/ -run TestParseWith_BinaryService -v +go test ./cmd/ -run TestParseWith_UnknownServiceMongodb -v +``` + +Expected: FAIL for the two binary-service tests — `services.Lookup("s3")` and `services.Lookup("mail")` return errors today, so `parseWith` rejects them. The `mongodb` test should already PASS since unknown is unknown. + +- [ ] **Step 3: Update `parseWith` in `cmd/install.go`** + +In `cmd/install.go`, replace the validation at line 61-63: + +Before: +```go + if _, err := services.Lookup(s.name); err != nil { + return spec, fmt.Errorf("unknown service %q in --with (available: %s)", s.name, strings.Join(services.Available(), ", ")) + } +``` + +After: +```go + if k, _, _, _ := services.LookupAny(s.name); k == services.KindUnknown { + return spec, fmt.Errorf("unknown service %q in --with (available: %s)", s.name, strings.Join(services.Available(), ", ")) + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +```bash +gofmt -w cmd/ +go vet ./cmd/ +go test ./cmd/ -run TestParseWith -v +go build ./... +``` + +Expected: PASS for all `TestParseWith_*` tests including the three new ones. + +- [ ] **Step 5: Run full test suite** + +```bash +go test ./... +``` + +Expected: every package passes. This is the last task — confirm the whole tree is clean before committing. + +- [ ] **Step 6: Commit** + +```bash +git add cmd/install.go cmd/install_test.go +git commit -m "install --with=service[…] accepts binary services + +parseWith now uses services.LookupAny, so --with=service[s3] and +--with=service[mail] no longer falsely error as 'unknown service'. +Validation behavior for genuinely unknown names is unchanged. + +Closes the last of the six silent failures from the mailpit PR review." +``` + +--- + +## Parallelization Guide + +Linear. Each task depends on Task 2 (`LookupAny` must exist before any callsite consumes it). After Task 2, Tasks 3-6 are technically independent and could run in parallel — but they all touch test files in `cmd/`, and a parallel `gofmt -w cmd/` could race with another task's edit. Safer to keep them sequential. + +Total: 6 commits, all on branch `feat/services-lookupany`. Whole branch should land in one PR. From eaca01d535c6977c008bf21289b953894a81f691 Mon Sep 17 00:00:00 2001 From: Clovis Muneza Date: Wed, 15 Apr 2026 02:59:13 -0400 Subject: [PATCH 3/8] Add services.LookupAny helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves a service name across both Docker and binary registries, returning a tagged union (Kind, BinaryService, Service, error). Binary wins on a hypothetical collision — matching the order used by the service:* command dispatcher's private resolveKind helper. Closes the gap that left several callsites only consulting the Docker registry and silently mishandling binary services (s3, mail). Per-callsite fixes follow in subsequent commits. --- internal/services/lookup.go | 44 +++++++++++++++ internal/services/lookup_test.go | 96 ++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 internal/services/lookup.go create mode 100644 internal/services/lookup_test.go diff --git a/internal/services/lookup.go b/internal/services/lookup.go new file mode 100644 index 0000000..081fc52 --- /dev/null +++ b/internal/services/lookup.go @@ -0,0 +1,44 @@ +package services + +import ( + "fmt" + "strings" +) + +// Kind classifies which registry a service name resolves to. +type Kind int + +const ( + KindUnknown Kind = iota + KindDocker + KindBinary +) + +// LookupAny resolves a service name across both the Docker and binary +// registries. +// +// Lookup order is binary first, then Docker — matching the convention used +// by the service:* command dispatcher (internal/commands/service/dispatch.go). +// A name found in only one registry returns that kind. A name in neither +// registry returns KindUnknown plus a non-nil error whose text matches the +// Lookup error format so callsites switching from Lookup retain the same +// error UX. +// +// When kind != KindUnknown, exactly one of binSvc / docSvc is non-nil. +// +// For service:* commands that also need to enforce a no-collision invariant +// against an active registry.Registry, see resolveKind in +// internal/commands/service/dispatch.go. +func LookupAny(name string) (kind Kind, binSvc BinaryService, docSvc Service, err error) { + if svc, ok := LookupBinary(name); ok { + return KindBinary, svc, nil, nil + } + if svc, lookupErr := Lookup(name); lookupErr == nil { + return KindDocker, nil, svc, nil + } + return KindUnknown, nil, nil, fmt.Errorf( + "unknown service %q (available: %s)", + name, + strings.Join(Available(), ", "), + ) +} diff --git a/internal/services/lookup_test.go b/internal/services/lookup_test.go new file mode 100644 index 0000000..6e06604 --- /dev/null +++ b/internal/services/lookup_test.go @@ -0,0 +1,96 @@ +package services + +import ( + "strings" + "testing" +) + +func TestLookupAny_BinaryService(t *testing.T) { + kind, binSvc, docSvc, err := LookupAny("mail") + if err != nil { + t.Fatalf("LookupAny(\"mail\") error = %v", err) + } + if kind != KindBinary { + t.Errorf("kind = %v, want KindBinary", kind) + } + if binSvc == nil { + t.Error("binSvc is nil; want non-nil for binary kind") + } + if docSvc != nil { + t.Errorf("docSvc = %#v, want nil for binary kind", docSvc) + } +} + +func TestLookupAny_DockerService(t *testing.T) { + kind, binSvc, docSvc, err := LookupAny("mysql") + if err != nil { + t.Fatalf("LookupAny(\"mysql\") error = %v", err) + } + if kind != KindDocker { + t.Errorf("kind = %v, want KindDocker", kind) + } + if docSvc == nil { + t.Error("docSvc is nil; want non-nil for docker kind") + } + if binSvc != nil { + t.Errorf("binSvc = %#v, want nil for docker kind", binSvc) + } +} + +func TestLookupAny_Unknown(t *testing.T) { + kind, binSvc, docSvc, err := LookupAny("mongodb") + if err == nil { + t.Fatal("LookupAny(\"mongodb\") error = nil; want non-nil for unknown name") + } + if kind != KindUnknown { + t.Errorf("kind = %v, want KindUnknown", kind) + } + if binSvc != nil || docSvc != nil { + t.Errorf("binSvc=%v docSvc=%v; want both nil", binSvc, docSvc) + } + if !strings.Contains(err.Error(), `unknown service "mongodb"`) { + t.Errorf("error %q missing expected text", err) + } + if !strings.Contains(err.Error(), "available:") { + t.Errorf("error %q missing available list", err) + } +} + +func TestLookupAny_BinaryWinsOnCollision(t *testing.T) { + // Pin the lookup-order invariant by temporarily seeding both registries + // with the same key. Restore via t.Cleanup so other tests are unaffected. + const key = "collisiontest" + + // Stash any pre-existing entries (defensive — there should be none). + prevBin, hadBin := binaryRegistry[key] + prevDoc, hadDoc := registry[key] + t.Cleanup(func() { + if hadBin { + binaryRegistry[key] = prevBin + } else { + delete(binaryRegistry, key) + } + if hadDoc { + registry[key] = prevDoc + } else { + delete(registry, key) + } + }) + + binaryRegistry[key] = &Mailpit{} // any BinaryService will do + registry[key] = &MySQL{} // any Service will do + + kind, binSvc, docSvc, err := LookupAny(key) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if kind != KindBinary { + t.Errorf("kind = %v, want KindBinary (binary should win on collision)", kind) + } + if binSvc == nil { + t.Error("binSvc is nil; want non-nil") + } + if docSvc != nil { + t.Errorf("docSvc = %#v, want nil (binary won)", docSvc) + } +} From 073df9309f42d331421374f9c86d3808ea9bcffe Mon Sep 17 00:00:00 2001 From: Clovis Muneza Date: Wed, 15 Apr 2026 03:03:43 -0400 Subject: [PATCH 4/8] setup.go: list binary services in wizard and provision them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracts buildServiceOptions() so the wizard's service multi-select includes both Docker and binary services (mail, s3). The post-wizard provisioning loop now branches on kind and calls service:add with the appropriate arg form for each kind — the binary branch uses the single-arg form which routes through resolveKind correctly. Fixes two of the six silent failures called out in the mailpit PR review. --- cmd/setup.go | 49 +++++++++++++++++++++++++++++++++++++---------- cmd/setup_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 10 deletions(-) create mode 100644 cmd/setup_test.go diff --git a/cmd/setup.go b/cmd/setup.go index 562cc9b..343802d 100644 --- a/cmd/setup.go +++ b/cmd/setup.go @@ -66,13 +66,7 @@ var setupCmd = &cobra.Command{ } // Service options. - var svcOpts []selectOption - for _, name := range services.Available() { - svc, _ := services.Lookup(name) - if svc != nil { - svcOpts = append(svcOpts, selectOption{label: svc.DisplayName(), value: name}) - } - } + svcOpts := buildServiceOptions() // Load settings, falling back to defaults on error. settings, err := config.LoadSettings() @@ -249,11 +243,20 @@ var setupCmd = &cobra.Command{ if len(selectedServices) > 0 { fmt.Fprintln(os.Stderr) for _, name := range selectedServices { - svc, _ := services.Lookup(name) - if svc == nil { + kind, _, docSvc, lookupErr := services.LookupAny(name) + if lookupErr != nil { + ui.Fail(fmt.Sprintf("Service %s: %v", name, lookupErr)) continue } - svcArgs := []string{name, svc.DefaultVersion()} + var svcArgs []string + switch kind { + case services.KindBinary: + // Binary services are unversioned; service:add ignores any + // explicit version (verified in Task 1). + svcArgs = []string{name} + case services.KindDocker: + svcArgs = []string{name, docSvc.DefaultVersion()} + } if err := service.RunAdd(svcArgs); err != nil { if !errors.Is(err, ui.ErrAlreadyPrinted) { ui.Fail(fmt.Sprintf("Service %s failed: %v", name, err)) @@ -271,3 +274,29 @@ var setupCmd = &cobra.Command{ func init() { rootCmd.AddCommand(setupCmd) } + +// buildServiceOptions returns the wizard's service multi-select options. +// Both Docker and binary services are listed using their DisplayName so that +// binary-only services (mail, s3) are visible in the picker. +func buildServiceOptions() []selectOption { + names := services.Available() + out := make([]selectOption, 0, len(names)) + for _, name := range names { + kind, binSvc, docSvc, err := services.LookupAny(name) + if err != nil { + // Available() is the union of both registries; LookupAny over + // the same names should never miss. If it does, skip silently + // rather than break the wizard. + continue + } + var label string + switch kind { + case services.KindBinary: + label = binSvc.DisplayName() + case services.KindDocker: + label = docSvc.DisplayName() + } + out = append(out, selectOption{label: label, value: name}) + } + return out +} diff --git a/cmd/setup_test.go b/cmd/setup_test.go new file mode 100644 index 0000000..9b11796 --- /dev/null +++ b/cmd/setup_test.go @@ -0,0 +1,44 @@ +package cmd + +import ( + "testing" +) + +func TestBuildServiceOptions_IncludesBothKinds(t *testing.T) { + opts := buildServiceOptions() + if len(opts) == 0 { + t.Fatal("buildServiceOptions() returned empty; want at least one option") + } + + // Pin specific names from each registry. mysql is Docker; mail and s3 are binary. + want := []string{"mysql", "postgres", "redis", "mail", "s3"} + for _, name := range want { + found := false + for _, opt := range opts { + if opt.value == name { + found = true + if opt.label == "" { + t.Errorf("option %q has empty label", name) + } + break + } + } + if !found { + t.Errorf("buildServiceOptions() missing %q", name) + } + } +} + +func TestBuildServiceOptions_LabelsUseDisplayName(t *testing.T) { + opts := buildServiceOptions() + for _, opt := range opts { + // DisplayName for mail should be "Mail (Mailpit)" — not "mail". + if opt.value == "mail" && opt.label != "Mail (Mailpit)" { + t.Errorf("mail label = %q, want %q", opt.label, "Mail (Mailpit)") + } + // DisplayName for s3 should be "S3 Storage (RustFS)". + if opt.value == "s3" && opt.label != "S3 Storage (RustFS)" { + t.Errorf("s3 label = %q, want %q", opt.label, "S3 Storage (RustFS)") + } + } +} From fdac7030d7e0f3e8c1702aeee3ed5f4283864c83 Mon Sep 17 00:00:00 2001 From: Clovis Muneza Date: Wed, 15 Apr 2026 03:07:45 -0400 Subject: [PATCH 5/8] service:env: print env vars for binary services correctly Extracts an envVarsFor helper that resolves the service name via LookupAny and dispatches to the right EnvVars signature for each kind (BinaryService.EnvVars(projectName) vs Service.EnvVars(projectName, port)). Both call sites in envCmd now go through it, eliminating the duplicated branching and giving us a real test surface. Previously "pv service:env mail" hard-errored and "pv service:env" with no args printed "Skipping unknown service" for each registered binary service. Fixes two of the six silent failures from the mailpit PR review. --- internal/commands/service/env.go | 28 +++++++++++++++--- internal/commands/service/env_test.go | 42 +++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 internal/commands/service/env_test.go diff --git a/internal/commands/service/env.go b/internal/commands/service/env.go index eb38d39..34d07d4 100644 --- a/internal/commands/service/env.go +++ b/internal/commands/service/env.go @@ -42,12 +42,11 @@ var envCmd = &cobra.Command{ fmt.Fprintln(os.Stderr) for key, instance := range svcs { svcName, _ := services.ParseServiceKey(key) - svc, err := services.Lookup(svcName) + envVars, err := envVarsFor(svcName, projectName, instance.Port) if err != nil { ui.Subtle(fmt.Sprintf("Skipping unknown service %q", svcName)) continue } - envVars := svc.EnvVars(projectName, instance.Port) printEnvVars(key, envVars) } return nil @@ -68,12 +67,11 @@ var envCmd = &cobra.Command{ } svcName, _ := services.ParseServiceKey(key) - svc, err := services.Lookup(svcName) + envVars, err := envVarsFor(svcName, projectName, instance.Port) if err != nil { return err } - envVars := svc.EnvVars(projectName, instance.Port) fmt.Fprintln(os.Stderr) printEnvVars(key, envVars) @@ -81,6 +79,28 @@ var envCmd = &cobra.Command{ }, } +// envVarsFor resolves a service name across both registries and returns the +// .env keys/values it injects into a linked project. Used by both the +// all-services and single-service code paths in envCmd so the per-kind +// dispatch lives in one place. +// +// Binary services have EnvVars(projectName) — port is fixed by the binary +// itself. Docker services have EnvVars(projectName, port) — port comes from +// the registry.ServiceInstance. +func envVarsFor(svcName, projectName string, port int) (map[string]string, error) { + kind, binSvc, docSvc, err := services.LookupAny(svcName) + if err != nil { + return nil, err + } + switch kind { + case services.KindBinary: + return binSvc.EnvVars(projectName), nil + case services.KindDocker: + return docSvc.EnvVars(projectName, port), nil + } + return nil, fmt.Errorf("unexpected kind %v for %q", kind, svcName) +} + func printEnvVars(key string, envVars map[string]string) { fmt.Fprintf(os.Stderr, " %s\n", ui.Muted.Render("# "+key)) for k, v := range envVars { diff --git a/internal/commands/service/env_test.go b/internal/commands/service/env_test.go new file mode 100644 index 0000000..251c6bb --- /dev/null +++ b/internal/commands/service/env_test.go @@ -0,0 +1,42 @@ +package service + +import ( + "strings" + "testing" +) + +func TestEnvVarsFor_BinaryService(t *testing.T) { + got, err := envVarsFor("mail", "anyproject", 0) + if err != nil { + t.Fatalf("envVarsFor(\"mail\") error = %v", err) + } + if got["MAIL_MAILER"] != "smtp" { + t.Errorf("MAIL_MAILER = %q, want smtp", got["MAIL_MAILER"]) + } + if got["MAIL_HOST"] != "127.0.0.1" { + t.Errorf("MAIL_HOST = %q, want 127.0.0.1", got["MAIL_HOST"]) + } +} + +func TestEnvVarsFor_DockerService(t *testing.T) { + // MySQL.EnvVars takes (projectName, port) and uses both. Pass a non-default + // port to verify the port arg is consulted — a regression that always + // passed 0 in the docker branch would silently set DB_PORT=0. + got, err := envVarsFor("mysql", "anyproject", 3306) + if err != nil { + t.Fatalf("envVarsFor(\"mysql\") error = %v", err) + } + if got["DB_PORT"] != "3306" { + t.Errorf("DB_PORT = %q, want 3306", got["DB_PORT"]) + } +} + +func TestEnvVarsFor_Unknown(t *testing.T) { + _, err := envVarsFor("mongodb", "anyproject", 0) + if err == nil { + t.Fatal("expected error for unknown service") + } + if !strings.Contains(err.Error(), `unknown service "mongodb"`) { + t.Errorf("error %q missing expected text", err) + } +} From 6f19458e9af9919e090b490512ff7825cb40e079 Mon Sep 17 00:00:00 2001 From: Clovis Muneza Date: Wed, 15 Apr 2026 03:12:13 -0400 Subject: [PATCH 6/8] doctor: skip binary services in per-service Docker check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doctor previously reported every registered binary service (mail, s3) as "unknown service type — registry may be out of date" because the loop fed every name through services.Lookup, which only consults the Docker registry. Binary services have first-class observability via pv service:list / pv service:status / pv service:logs; doctor's job is cross-cutting infra health, not per-binary-service supervision health. Fixes one of the six silent failures from the mailpit PR review. --- cmd/doctor.go | 10 ++++++++ cmd/doctor_test.go | 58 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/cmd/doctor.go b/cmd/doctor.go index efbe076..41eff02 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -616,6 +616,16 @@ func runServiceChecks(reg *registry.Registry) sectionResult { for key, svc := range svcs { svcName, version := services.ParseServiceKey(key) + // Skip binary services: they have no Docker container to probe. + // Binary supervision health is reported via `pv service:list` and + // `pv service:status`, which read the daemon's status snapshot + // directly. Including them here would couple doctor to the daemon's + // internal status format and produce misleading "lookup_error" + // output for healthy services (mail, s3). + if kind, _, _, _ := services.LookupAny(svcName); kind == services.KindBinary { + continue + } + status := "unknown" if engine != nil { svcDef, lookupErr := services.Lookup(svcName) diff --git a/cmd/doctor_test.go b/cmd/doctor_test.go index bbea315..fa00e19 100644 --- a/cmd/doctor_test.go +++ b/cmd/doctor_test.go @@ -3,6 +3,7 @@ package cmd import ( "os" "path/filepath" + "strings" "testing" "github.com/prvious/pv/internal/config" @@ -103,3 +104,60 @@ func TestDoctor_WithValidProject(t *testing.T) { cmd.SetArgs([]string{"doctor"}) _ = cmd.Execute() } + +// TestDoctor_SkipsBinaryServices verifies that registering a binary service +// does NOT add a "lookup_error" / "unknown service type" entry to doctor's +// output. Binary services have their own observability and are intentionally +// skipped by the doctor's Docker-container check. +func TestDoctor_SkipsBinaryServices(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + if err := config.EnsureDirs(); err != nil { + t.Fatalf("EnsureDirs() error = %v", err) + } + + // Register a binary service in the registry on disk. + reg, err := registry.Load() + if err != nil { + t.Fatalf("registry.Load() error = %v", err) + } + tru := true + reg.Services["mail"] = ®istry.ServiceInstance{ + Kind: "binary", + Port: 1025, + Enabled: &tru, + } + if err := reg.Save(); err != nil { + t.Fatalf("reg.Save() error = %v", err) + } + + // Capture stderr output from doctor. + r, w, pipeErr := os.Pipe() + if pipeErr != nil { + t.Fatalf("os.Pipe() error = %v", pipeErr) + } + prevStderr := os.Stderr + os.Stderr = w + + cmd := newDoctorCmd() + cmd.SetArgs([]string{"doctor"}) + _ = cmd.Execute() + + w.Close() + os.Stderr = prevStderr + + buf := make([]byte, 64*1024) + n, _ := r.Read(buf) + output := string(buf[:n]) + + if strings.Contains(output, "lookup_error") { + t.Errorf("doctor output should not contain \"lookup_error\" for binary service; got:\n%s", output) + } + if strings.Contains(output, "unknown service type") { + t.Errorf("doctor output should not contain \"unknown service type\" for binary service; got:\n%s", output) + } + if strings.Contains(output, "registry may be out of date") { + t.Errorf("doctor output should not contain \"registry may be out of date\" for binary service; got:\n%s", output) + } +} From 84b3c5d02a14b2ebe68b0d0a835d0a0d332b7502 Mon Sep 17 00:00:00 2001 From: Clovis Muneza Date: Wed, 15 Apr 2026 03:16:19 -0400 Subject: [PATCH 7/8] =?UTF-8?q?install=20--with=3Dservice[=E2=80=A6]=20acc?= =?UTF-8?q?epts=20binary=20services?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseWith now uses services.LookupAny, so --with=service[s3] and --with=service[mail] no longer falsely error as 'unknown service'. Validation behavior for genuinely unknown names is unchanged. Closes the last of the six silent failures from the mailpit PR review. --- cmd/install.go | 2 +- cmd/install_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/cmd/install.go b/cmd/install.go index 40e2c26..43ba9c4 100644 --- a/cmd/install.go +++ b/cmd/install.go @@ -58,7 +58,7 @@ func parseWith(raw string) (withSpec, error) { if len(parts) > 1 { s.version = parts[1] } - if _, err := services.Lookup(s.name); err != nil { + if k, _, _, _ := services.LookupAny(s.name); k == services.KindUnknown { return spec, fmt.Errorf("unknown service %q in --with (available: %s)", s.name, strings.Join(services.Available(), ", ")) } spec.services = append(spec.services, s) diff --git a/cmd/install_test.go b/cmd/install_test.go index 4f2db29..9be2528 100644 --- a/cmd/install_test.go +++ b/cmd/install_test.go @@ -3,6 +3,7 @@ package cmd import ( "os" "path/filepath" + "strings" "testing" "github.com/spf13/cobra" @@ -160,6 +161,42 @@ func TestParseWith_ServiceNoVersion(t *testing.T) { } } +func TestParseWith_BinaryServiceS3(t *testing.T) { + spec, err := parseWith("service[s3]") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(spec.services) != 1 { + t.Fatalf("expected 1 service, got %d", len(spec.services)) + } + if spec.services[0].name != "s3" { + t.Errorf("service[0].name = %q, want s3", spec.services[0].name) + } +} + +func TestParseWith_BinaryServiceMail(t *testing.T) { + spec, err := parseWith("service[mail]") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(spec.services) != 1 { + t.Fatalf("expected 1 service, got %d", len(spec.services)) + } + if spec.services[0].name != "mail" { + t.Errorf("service[0].name = %q, want mail", spec.services[0].name) + } +} + +func TestParseWith_UnknownServiceMongodb(t *testing.T) { + _, err := parseWith("service[mongodb]") + if err == nil { + t.Fatal("expected error for unknown service") + } + if !strings.Contains(err.Error(), `unknown service "mongodb"`) { + t.Errorf("error %q missing expected text", err) + } +} + func TestInstallCmd_AlreadyInstalled(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) From 09b925168b865680109f30ea41136eecef5230f5 Mon Sep 17 00:00:00 2001 From: Clovis Muneza Date: Wed, 15 Apr 2026 03:23:59 -0400 Subject: [PATCH 8/8] TestDoctor_SkipsBinaryServices: add Docker-less regression assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-existing absence-of-error-strings checks only fire when Docker is reachable, because the lookup_error branch in cmd/doctor.go is gated behind `engine != nil`. In CI environments without Docker (or local runs without Colima), the test passed regardless of whether the fix in 6f19458 was applied — empirically verified by reverting the fix and seeing the test still pass. Add a positive assertion that the output does not suggest "pv service:start mail" — pre-fix code surfaces mail as a failing check with that Fix message even when engine is nil. Confirmed catches the regression: with the fix temporarily reverted, the test now FAILS. --- cmd/doctor_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmd/doctor_test.go b/cmd/doctor_test.go index fa00e19..b4246d8 100644 --- a/cmd/doctor_test.go +++ b/cmd/doctor_test.go @@ -160,4 +160,12 @@ func TestDoctor_SkipsBinaryServices(t *testing.T) { if strings.Contains(output, "registry may be out of date") { t.Errorf("doctor output should not contain \"registry may be out of date\" for binary service; got:\n%s", output) } + // The three asserts above only fire when Docker is reachable (the + // lookup_error branch is gated behind engine != nil). In a Docker-less + // test env, pre-fix code would still surface mail as a failing check + // whose Fix line reads "pv service:start mail". This positive assertion + // catches the regression in either environment. + if strings.Contains(output, "pv service:start mail") { + t.Errorf("doctor output should not suggest restarting binary service mail; got:\n%s", output) + } }