From a42fbdee950a0d35fa4b31f85a9cbd6a0851ad5a Mon Sep 17 00:00:00 2001 From: Gustavo Bertoi Date: Wed, 1 Jul 2026 00:24:11 -0300 Subject: [PATCH] feat(doctor): real safe --fix remediations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the no-op `--fix` stub with real, safe, idempotent, reversible remediations gated per failing probe (spec 13): - net.shared: create the missing tool-owned external bridge network via docker.EnsureNetwork under the flock (compose refuses external nets). - state.refs: prune stale ledger ref rows for projects with no live container via workspace.Reconcile (locks internally; derived rows only). - fs.xdg: create missing / tighten group-or-world-writable XDG dirs to 0700. - dns.resolver / trust.host: diagnose-only — the sudo /etc/hosts write and the mkcert/NSS trust-store drive are out of --fix by construction; each keeps its manual remediation. Each probe now carries Fixable + a fix + recheck; applyFixes runs ONLY fixable, non-OK probes, re-probes, and reports fixed/still-failing, updating the report + exit code to the post-fix state. A passing check's fix is never invoked; nothing destructive (volume/container/network/db/CA removal) is ever run. docker.Check gains additive id/category/fixable/fixed JSON fields; the existing {"checks":[...]} envelope is preserved and gains a "fixes" key under --fix. --quiet now prints only non-OK lines. Table-driven tests: failing-then-fixed, non-fixable-left-with-remediation, passing-fix-never-run, failing-fix-still-failing, mixed matrix, plus wiring tests for the real net.shared / state.refs / fs.xdg fixes and dirSecure. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/cli/doctor.go | 638 +++++++++++++++++++++++++------- internal/cli/doctor_fix_test.go | 330 +++++++++++++++++ internal/docker/preflight.go | 10 + 3 files changed, 854 insertions(+), 124 deletions(-) create mode 100644 internal/cli/doctor_fix_test.go diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 668aabc..9851647 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -1,21 +1,34 @@ package cli import ( + "context" "encoding/json" "fmt" "os" + "path/filepath" "github.com/spf13/cobra" "github.com/open-source-cloud/devstack/internal/config" "github.com/open-source-cloud/devstack/internal/dns" "github.com/open-source-cloud/devstack/internal/docker" + "github.com/open-source-cloud/devstack/internal/generate" + "github.com/open-source-cloud/devstack/internal/lock" "github.com/open-source-cloud/devstack/internal/proxy" "github.com/open-source-cloud/devstack/internal/state" "github.com/open-source-cloud/devstack/internal/trust" + "github.com/open-source-cloud/devstack/internal/workspace" "github.com/open-source-cloud/devstack/internal/xdg" ) +// Probe categories (spec 13): a `fail` in critical → non-zero exit; warn/info do +// not gate. Category is orthogonal to Status. +const ( + catCritical = "critical" + catWarn = "warn" + catInfo = "info" +) + func newDoctorCmd(g *GlobalOpts) *cobra.Command { var ( fix bool @@ -26,6 +39,11 @@ func newDoctorCmd(g *GlobalOpts) *cobra.Command { Short: "Probe the environment and report capabilities with remediations", Long: "doctor runs the REAL branch logic (not docs) for the tools and paths devstack\n" + "depends on, and prints a one-line remediation for anything that isn't OK.\n\n" + + "With --fix it applies the STRICTLY non-destructive, idempotent remediations\n" + + "(create the shared external network, prune stale ledger ref rows, create/tighten\n" + + "XDG dirs) under the machine-global lock, re-probes, and reports fixed/still-failing.\n" + + "It never removes a volume, container, network, database, or CA — those live in\n" + + "the teardown verbs (`workspace destroy`, `uninstall`).\n\n" + "With --rebuild-state, the shared_service + ref ledger is reconstructed from\n" + "on-disk config intersected with live container labels (recovery when state.db\n" + "is lost or corrupt — the ledger is a cache of reality).", @@ -34,183 +52,530 @@ func newDoctorCmd(g *GlobalOpts) *cobra.Command { if rebuildState { return rebuildLedger(cmd, g) } - checks := runDoctor(cmd) + + sess, cleanup := openDoctorSession(cmd) + defer cleanup() + + probes := sess.probes(cmd.Context()) + + var fixes []fixResult + if fix { + // applyFixes runs only fixable, non-OK probes under the flock (inside + // each fix closure), re-probes, and updates probes in place so the + // report and exit code below reflect the POST-fix state. + fixes = applyFixes(cmd.Context(), probes) + } + + checks := checksOf(probes) if g.JSON { - return json.NewEncoder(cmd.OutOrStdout()).Encode(map[string]any{"checks": checks}) + payload := map[string]any{"checks": checks} + if fix { + payload["fixes"] = fixes + } + return json.NewEncoder(cmd.OutOrStdout()).Encode(payload) } - renderChecks(cmd, checks) + renderChecks(cmd, checks, g.Quiet) if fix { - doctorFix(cmd) + renderFixes(cmd, fixes) } - for _, c := range checks { - if c.Status == docker.StatusFail { - return fmt.Errorf("doctor found %d failing check(s)", countFails(checks)) - } + if n := countFails(checks); n > 0 { + return fmt.Errorf("doctor found %d failing check(s)", n) } return nil }, } - cmd.Flags().BoolVar(&fix, "fix", false, "apply safe automatic remediations (M6)") + cmd.Flags().BoolVar(&fix, "fix", false, "apply safe, idempotent, non-destructive remediations under the lock") cmd.Flags().BoolVar(&rebuildState, "rebuild-state", false, "reconstruct the ledger from config + live container labels") return cmd } -// doctorFix applies the STRICTLY non-destructive remediations (spec 13): the -// self-healing ledger reconcile (prune ref rows for projects no longer live). It -// never drops a volume/DB/container — those are explicit-confirmation jobs -// (`shared gc`, `workspace destroy`). Best-effort: a down daemon or no workspace -// is reported, not fatal. -func doctorFix(cmd *cobra.Command) { - w := cmd.OutOrStdout() - fmt.Fprintln(w, "\n--fix: applying safe remediations…") - mgr, closeFn, err := buildManager(cmd) - if err != nil { - fmt.Fprintf(w, " reconcile skipped: %v\n", err) - return +// probe is one doctor capability check plus its optional safe remediation. The +// embedded Check carries the JSON-serialized result (id/category/status/detail/ +// remediation/fixable). fix, when set (and only when check.Fixable), performs a +// reconstructible, non-destructive repair; recheck re-runs just this probe after +// a fix so `--fix` can report the post-fix state. A probe with a nil fix is +// diagnose-only and is left with its remediation for the human to run. +type probe struct { + check docker.Check + fix func(context.Context) error + recheck func(context.Context) docker.Check +} + +// fixResult records the outcome of one attempted `--fix` remediation. +type fixResult struct { + ID string `json:"id"` + Fixed bool `json:"fixed"` + Detail string `json:"detail,omitempty"` + Remediation string `json:"remediation,omitempty"` +} + +// applyFixes runs the remediation of every FIXABLE, non-OK probe, then re-probes +// it and records whether it is now green. Invariants (spec 13 safe subset): +// - a passing (StatusOK) probe's fix is NEVER invoked; +// - a probe with no fix (not remediable) is left untouched with its remediation; +// - each probe's post-fix Check replaces the pre-fix one in `probes`, so the +// final report and exit code reflect the POST-fix state. +// +// Shared-state mutations are locked INSIDE the individual fix closures (network +// ensure and ref reconcile each take the flock); filesystem-only fixes (XDG dir +// perms) need no lock. This keeps the runner itself pure and unit-testable. +func applyFixes(ctx context.Context, probes []probe) []fixResult { + var results []fixResult + for i := range probes { + p := &probes[i] + // Never touch a passing check, and skip diagnose-only probes. + if p.check.Status == docker.StatusOK || !p.check.Fixable || p.fix == nil { + continue + } + res := fixResult{ID: p.check.ID} + if err := p.fix(ctx); err != nil { + res.Detail = "fix failed: " + err.Error() + res.Remediation = p.check.Remediation + results = append(results, res) + continue + } + if p.recheck != nil { + p.check = p.recheck(ctx) + } + p.check.Fixed = p.check.Status == docker.StatusOK + res.Fixed = p.check.Fixed + res.Detail = p.check.Detail + if !res.Fixed { + res.Remediation = p.check.Remediation + } + results = append(results, res) } - defer closeFn() - pruned, err := mgr.Reconcile(cmd.Context()) - if err != nil { - fmt.Fprintf(w, " reconcile skipped: %v\n", err) - return + return results +} + +// checksOf projects the probe slice down to the serialized checks. +func checksOf(probes []probe) []docker.Check { + out := make([]docker.Check, 0, len(probes)) + for _, p := range probes { + out = append(out, p.check) } - fmt.Fprintf(w, " reconciled the ledger: pruned %d stale ref row(s)\n", len(pruned)) + return out } -// rebuildLedger reconstructs the shared_service + ref ledger from on-disk config -// intersected with live container labels (spec 09 §crash-recovery). -func rebuildLedger(cmd *cobra.Command, g *GlobalOpts) error { - mgr, closeFn, err := buildManager(cmd) - if err != nil { - return err +// doctorSession holds the best-effort live resources the probes read from and +// that `--fix` mutates: the read-only docker client, the state ledger, the loaded +// workspace, and the machine-global lock path. Any handle may be nil/absent when +// unavailable (no daemon, corrupt ledger, cwd not a workspace) — probes degrade +// to a diagnose-only result rather than crashing, and their fixes become no-ops. +type doctorSession struct { + cwd string + client docker.Client // nil when the Engine SDK client could not be built + clientErr error + db *state.DB // nil when the ledger could not be opened + dbErr error + model *config.Model // nil when cwd is not a workspace + ctxName string + lockPath string +} + +// openDoctorSession opens the docker client and state ledger once (best-effort) +// so probes and their fixes share a single set of handles for the whole run. The +// returned cleanup releases them. +func openDoctorSession(cmd *cobra.Command) (*doctorSession, func()) { + ctx := cmd.Context() + cwd, _ := os.Getwd() + s := &doctorSession{ + cwd: cwd, + ctxName: state.DefaultContext, + lockPath: filepath.Join(xdg.RuntimeDir(), "devstack.lock"), } - defer closeFn() - sum, err := mgr.RebuildState(cmd.Context()) - if err != nil { - return err + if m, err := config.Load(cwd); err == nil { + s.model = m } - if g.JSON { - return writeJSON(cmd, sum) + if c, err := docker.NewClient(ctx); err == nil { + s.client = c + s.ctxName = c.ContextName() + } else { + s.clientErr = err + } + if db, err := state.Open(ctx, xdg.DataHome(), s.ctxName); err == nil { + s.db = db + } else { + s.dbErr = err + } + return s, func() { + if s.db != nil { + s.db.Close() + } + if s.client != nil { + _ = s.client.Close() + } } - fmt.Fprintf(cmd.OutOrStdout(), - "rebuilt ledger from live labels: %d shared service(s), %d ref row(s)\n", - len(sum.Shared), sum.Refs) - return nil } -// runDoctor assembles the full capability matrix. Each probe is independent so a -// single failure never hides the others. -func runDoctor(cmd *cobra.Command) []docker.Check { - ctx := cmd.Context() - var checks []docker.Check +// manager builds a workspace.Manager over the session's shared handles. Reconcile +// (the state.refs fix) uses only DB/Docker/LockPath, so a nil Model is fine. +func (s *doctorSession) manager() *workspace.Manager { + return &workspace.Manager{ + Model: s.model, + DB: s.db, + Docker: s.client, + LockPath: s.lockPath, + } +} - // Working-directory safety (WSL2 /mnt refusal). - cwd, _ := os.Getwd() - if err := xdg.RefuseWindowsMount(cwd); err != nil { - checks = append(checks, docker.Check{ - Name: "working dir", Status: docker.StatusFail, Detail: err.Error(), +// probes assembles the full capability matrix. Each probe is independent so a +// single failure never hides the others; the fixable ones carry a safe fix. +func (s *doctorSession) probes(ctx context.Context) []probe { + var probes []probe + + // fs.workdir — WSL2 /mnt refusal (critical). + if err := xdg.RefuseWindowsMount(s.cwd); err != nil { + probes = append(probes, plain(docker.Check{ + Name: "working dir", ID: "fs.workdir", Category: catCritical, + Status: docker.StatusFail, Detail: err.Error(), Remediation: "move the workspace onto the Linux filesystem", - }) + })) } else { - checks = append(checks, docker.Check{Name: "working dir", Status: docker.StatusOK, Detail: cwd}) + probes = append(probes, plain(docker.Check{ + Name: "working dir", ID: "fs.workdir", Category: catCritical, + Status: docker.StatusOK, Detail: s.cwd, + })) } - // State dir filesystem (SQLite reliability) and lock dir filesystem (flock - // reliability) — they can be on different filesystems, so probe both. + // fs.statedir / fs.lockdir — SQLite + flock reliability (warn on 9p/networked). stateDir := xdg.DataHome() - checks = append(checks, fsCheck("state dir (SQLite)", stateDir, - "set XDG_DATA_HOME to a local (ext4/apfs) path on the Linux filesystem")) + probes = append(probes, plain(fsCheck("state dir (SQLite)", "fs.statedir", stateDir, + "set XDG_DATA_HOME to a local (ext4/apfs) path on the Linux filesystem"))) lockDir := xdg.RuntimeDir() if lockDir != stateDir { - checks = append(checks, fsCheck("lock dir (flock)", lockDir, - "set XDG_RUNTIME_DIR to a local tmpfs/ext4 path; the advisory lock lives here")) + probes = append(probes, plain(fsCheck("lock dir (flock)", "fs.lockdir", lockDir, + "set XDG_RUNTIME_DIR to a local tmpfs/ext4 path; the advisory lock lives here"))) } - // WSL2 awareness (informational). + // fs.xdg — the private XDG dirs exist with 0700-ish perms (fixable: create/tighten). + probes = append(probes, s.xdgDirsProbe()) + + // platform — WSL2 awareness (info). if xdg.IsWSL2() { - checks = append(checks, docker.Check{Name: "platform", Status: docker.StatusOK, Detail: "WSL2 detected"}) + probes = append(probes, plain(docker.Check{ + Name: "platform", ID: "platform", Category: catInfo, + Status: docker.StatusOK, Detail: "WSL2 detected", + })) } - // Docker / compose / git preflight. - client, err := docker.NewClient(ctx) - if err != nil { - checks = append(checks, docker.Preflight(ctx, nil)...) - checks = append(checks, docker.Check{ - Name: "docker client", Status: docker.StatusWarn, Detail: err.Error(), + // Docker / compose / git preflight (critical). Categorized post-hoc. + if s.client == nil { + for _, c := range docker.Preflight(ctx, nil) { + probes = append(probes, plain(withCategory(c, catCritical))) + } + probes = append(probes, plain(docker.Check{ + Name: "docker client", ID: "docker.client", Category: catCritical, + Status: docker.StatusWarn, Detail: errText(s.clientErr), Remediation: "ensure DOCKER_HOST / the active docker context is valid", - }) + })) } else { - defer client.Close() - checks = append(checks, docker.Preflight(ctx, client)...) + for _, c := range docker.Preflight(ctx, s.client) { + probes = append(probes, plain(withCategory(c, catCritical))) + } } - // State ledger opens (and migrates) cleanly. - ctxName := state.DefaultContext - if client != nil { - ctxName = client.ContextName() - } - if db, err := state.Open(ctx, stateDir, ctxName); err != nil { - checks = append(checks, docker.Check{ - Name: "state ledger", Status: docker.StatusFail, Detail: err.Error(), - Remediation: "remove a corrupt state.db (a backup is kept) or run `devstack doctor --rebuild-state` (M2+)", - }) + // state.ledger — the ledger opens/migrates cleanly (critical). + if s.dbErr != nil { + probes = append(probes, plain(docker.Check{ + Name: "state ledger", ID: "state.ledger", Category: catCritical, + Status: docker.StatusFail, Detail: s.dbErr.Error(), + Remediation: "remove a corrupt state.db (a backup is kept) or run `devstack doctor --rebuild-state`", + })) } else { - v, _ := db.SchemaVersion() - checks = append(checks, docker.Check{Name: "state ledger", Status: docker.StatusOK, Detail: fmt.Sprintf("schema v%d @ %s", v, ctxName)}) - // Shared-service ledger summary (informational): instances + total refs. - shared, _ := db.ListSharedServices() + v, _ := s.db.SchemaVersion() + probes = append(probes, plain(docker.Check{ + Name: "state ledger", ID: "state.ledger", Category: catCritical, + Status: docker.StatusOK, Detail: fmt.Sprintf("schema v%d @ %s", v, s.ctxName), + })) + // state.shared — informational instance/ref summary (kept for compatibility). + shared, _ := s.db.ListSharedServices() totalRefs := 0 - for _, s := range shared { - n, _ := db.RefCount(s.Name) + for _, sv := range shared { + n, _ := s.db.RefCount(sv.Name) totalRefs += n } - db.Close() - checks = append(checks, docker.Check{ - Name: "shared services", Status: docker.StatusOK, + probes = append(probes, plain(docker.Check{ + Name: "shared services", ID: "state.shared", Category: catInfo, + Status: docker.StatusOK, Detail: fmt.Sprintf("%d instance(s), %d ref(s)", len(shared), totalRefs), - }) + })) } - // DNS entries for *.localhost (spec 05) — best-effort, only when the current - // dir is a workspace with a configured proxy. Never fatal (opt-in). - if m, err := config.Load(cwd); err == nil && proxy.Enabled(m) { - var hosts []string - for _, r := range proxy.BuildRoutes(m) { - hosts = append(hosts, r.Host) - } - if missing, err := dns.Missing(dns.DefaultHostsPath, hosts); err == nil { - if len(missing) == 0 { - checks = append(checks, docker.Check{Name: "dns (/etc/hosts)", Status: docker.StatusOK, Detail: fmt.Sprintf("%d *.localhost host(s) resolved", len(hosts))}) - } else { - checks = append(checks, docker.Check{ - Name: "dns (/etc/hosts)", Status: docker.StatusWarn, - Detail: fmt.Sprintf("%d of %d *.localhost host(s) missing", len(missing), len(hosts)), - Remediation: "run `sudo devstack dns setup`", + // net.shared — the tool-owned external bridge exists (fixable: EnsureNetwork). + probes = append(probes, s.netSharedProbe(ctx)) + + // state.refs — stale ref rows vs live containers (fixable: Reconcile). + probes = append(probes, s.stateRefsProbe(ctx)) + + // dns.resolver — *.localhost /etc/hosts fence, when the cwd is a proxied + // workspace. Diagnose-only: the write needs sudo, so it is out of `--fix`. + if p, ok := s.dnsProbe(); ok { + probes = append(probes, p) + } + + // trust.host — local-CA readiness (diagnose-only: driving mkcert/NSS is out of + // `--fix`; it mutates OS/browser trust stores which is a `trust install` job). + probes = append(probes, s.trustProbe(ctx)) + + return probes +} + +// netSharedProbe checks the pinned external bridge network exists; when it does +// not (and docker is reachable) the fix idempotently creates it under the lock. +// Classified `warn` (not critical): a fresh machine has no network until the +// first `up`, so a missing network must not gate the exit code — `--fix` (or the +// next `up`) creates it. +func (s *doctorSession) netSharedProbe(ctx context.Context) probe { + c := docker.Check{Name: "shared network", ID: "net.shared", Category: catWarn} + if s.client == nil { + c.Status = docker.StatusWarn + c.Detail = "docker unreachable; cannot inspect " + generate.SharedNetwork + c.Remediation = "start Docker, then re-run `devstack doctor`" + return plain(c) + } + exists, err := s.client.NetworkExists(ctx, generate.SharedNetwork) + switch { + case err != nil: + c.Status = docker.StatusWarn + c.Detail = err.Error() + c.Remediation = "verify the active docker context, then `devstack doctor --fix`" + return plain(c) + case exists: + c.Status = docker.StatusOK + c.Detail = generate.SharedNetwork + " present" + return plain(c) + default: + c.Status = docker.StatusWarn + c.Fixable = true + c.Detail = "network " + generate.SharedNetwork + " not found" + c.Remediation = "run `devstack doctor --fix` to create the external network" + } + return probe{ + check: c, + fix: func(ctx context.Context) error { + return lock.WithLock(ctx, s.lockPath, func() error { + return s.client.EnsureNetwork(ctx, generate.SharedNetwork, map[string]string{ + generate.LabelManaged: "true", }) - } + }) + }, + recheck: func(ctx context.Context) docker.Check { return s.netSharedProbe(ctx).check }, + } +} + +// stateRefsProbe detects ledger ref rows whose project has no live container and, +// when any exist, fixes them via the self-healing reconcile (which takes the +// flock internally and prunes only derived rows — never data). +func (s *doctorSession) stateRefsProbe(ctx context.Context) probe { + c := docker.Check{Name: "ledger refs", ID: "state.refs", Category: catWarn} + switch { + case s.db == nil: + c.Status = docker.StatusWarn + c.Detail = "state ledger unavailable; cannot reconcile ref rows" + c.Remediation = "resolve the state-ledger probe first" + return plain(c) + case s.client == nil: + c.Status = docker.StatusWarn + c.Detail = "docker unreachable; cannot compare ref rows against live containers" + c.Remediation = "start Docker, then `devstack doctor --fix`" + return plain(c) + } + stale, err := s.staleRefs(ctx) + if err != nil { + c.Status = docker.StatusWarn + c.Detail = err.Error() + c.Remediation = "verify the docker context and ledger" + return plain(c) + } + if len(stale) == 0 { + c.Status = docker.StatusOK + c.Detail = "no stale ref rows" + return plain(c) + } + c.Status = docker.StatusWarn + c.Fixable = true + c.Detail = fmt.Sprintf("%d stale ref row(s) for project(s) with no live container", len(stale)) + c.Remediation = "run `devstack doctor --fix` to prune stale ref rows" + mgr := s.manager() + return probe{ + check: c, + fix: func(ctx context.Context) error { + _, err := mgr.Reconcile(ctx) // takes the flock internally + return err + }, + recheck: func(ctx context.Context) docker.Check { return s.stateRefsProbe(ctx).check }, + } +} + +// staleRefs returns the ledger ref rows whose project has no running container +// (the label-filtered live set, All=true / one-offs excluded per ListManaged). +func (s *doctorSession) staleRefs(ctx context.Context) ([]state.Ref, error) { + containers, err := s.client.ListManaged(ctx, map[string]string{generate.LabelManaged: "true"}) + if err != nil { + return nil, fmt.Errorf("list managed containers: %w", err) + } + live := map[string]bool{} + for _, ct := range containers { + if p := ct.Labels[generate.LabelProject]; p != "" && ct.Running() { + live[p] = true + } + } + refs, err := s.db.AllRefs() + if err != nil { + return nil, err + } + var stale []state.Ref + for _, r := range refs { + if !live[r.Project] { + stale = append(stale, r) + } + } + return stale, nil +} + +// xdgDirsProbe verifies the private XDG dirs exist and are not group/other +// writable (SQLite ledger + flock integrity). The fix creates missing dirs and +// tightens perms to 0700 — reconstructible and non-destructive. +func (s *doctorSession) xdgDirsProbe() probe { + c := docker.Check{Name: "xdg dirs", ID: "fs.xdg", Category: catWarn} + dirs := []string{xdg.DataHome(), xdg.StateHome(), xdg.ConfigHome()} + var bad []string + for _, d := range dirs { + if !dirSecure(d) { + bad = append(bad, d) } } + if len(bad) == 0 { + c.Status = docker.StatusOK + c.Detail = "data/state/config dirs present (0700)" + return plain(c) + } + c.Status = docker.StatusWarn + c.Fixable = true + c.Detail = fmt.Sprintf("%d XDG dir(s) missing or group/world-writable", len(bad)) + c.Remediation = "run `devstack doctor --fix` to create them with 0700 permissions" + return probe{ + check: c, + fix: func(context.Context) error { + for _, d := range bad { + if err := os.MkdirAll(d, 0o700); err != nil { + return fmt.Errorf("create %s: %w", d, err) + } + if err := os.Chmod(d, 0o700); err != nil { + return fmt.Errorf("chmod %s: %w", d, err) + } + } + return nil + }, + recheck: func(context.Context) docker.Check { return s.xdgDirsProbe().check }, + } +} + +// dnsProbe reports whether the *.localhost /etc/hosts fence covers a proxied +// workspace's routes. Diagnose-only (the write needs sudo) — the second return +// is false when the cwd is not a proxied workspace. +func (s *doctorSession) dnsProbe() (probe, bool) { + if s.model == nil || !proxy.Enabled(s.model) { + return probe{}, false + } + var hosts []string + for _, r := range proxy.BuildRoutes(s.model) { + hosts = append(hosts, r.Host) + } + c := docker.Check{Name: "dns (/etc/hosts)", ID: "dns.resolver", Category: catWarn} + missing, err := dns.Missing(dns.DefaultHostsPath, hosts) + if err != nil { + c.Status = docker.StatusWarn + c.Detail = err.Error() + c.Remediation = "run `sudo devstack dns setup`" + return plain(c), true + } + if len(missing) == 0 { + c.Status = docker.StatusOK + c.Detail = fmt.Sprintf("%d *.localhost host(s) resolved", len(hosts)) + return plain(c), true + } + c.Status = docker.StatusWarn + c.Detail = fmt.Sprintf("%d of %d *.localhost host(s) missing", len(missing), len(hosts)) + c.Remediation = "run `sudo devstack dns setup` (a marker-fenced /etc/hosts write needs root)" + return plain(c), true +} - // Local-CA trust (spec 05) — opt-in, so never fatal: report readiness as a - // warning with the exact remediation when not fully set up. +// trustProbe reports local-CA readiness. Diagnose-only: driving mkcert/NSS +// mutates OS/browser trust stores, which is `trust install`, not `--fix`. +func (s *doctorSession) trustProbe(ctx context.Context) probe { ts := trust.New().Status(ctx) if ts.OK() { - checks = append(checks, docker.Check{Name: "trust (mkcert)", Status: docker.StatusOK, Detail: "local CA installed (" + ts.CARoot + ")"}) - } else { - checks = append(checks, docker.Check{ - Name: "trust (mkcert)", - Status: docker.StatusWarn, - Detail: fmt.Sprintf("mkcert=%v CA=%v firefox=%v", ts.MkcertFound, ts.CAInstalled, ts.FirefoxTrust), - Remediation: ts.Remediation, + return plain(docker.Check{ + Name: "trust (mkcert)", ID: "trust.host", Category: catWarn, + Status: docker.StatusOK, Detail: "local CA installed (" + ts.CARoot + ")", }) } + return plain(docker.Check{ + Name: "trust (mkcert)", ID: "trust.host", Category: catWarn, + Status: docker.StatusWarn, + Detail: fmt.Sprintf("mkcert=%v CA=%v firefox=%v", ts.MkcertFound, ts.CAInstalled, ts.FirefoxTrust), + Remediation: ts.Remediation, + }) +} + +// rebuildLedger reconstructs the shared_service + ref ledger from on-disk config +// intersected with live container labels (spec 09 §crash-recovery). +func rebuildLedger(cmd *cobra.Command, g *GlobalOpts) error { + mgr, closeFn, err := buildManager(cmd) + if err != nil { + return err + } + defer closeFn() + sum, err := mgr.RebuildState(cmd.Context()) + if err != nil { + return err + } + if g.JSON { + return writeJSON(cmd, sum) + } + fmt.Fprintf(cmd.OutOrStdout(), + "rebuilt ledger from live labels: %d shared service(s), %d ref row(s)\n", + len(sum.Shared), sum.Refs) + return nil +} + +// plain wraps a diagnose-only check (no fix) as a probe. +func plain(c docker.Check) probe { return probe{check: c} } + +// withCategory stamps a category onto a check that lacks one (e.g. from Preflight). +func withCategory(c docker.Check, cat string) docker.Check { + if c.Category == "" { + c.Category = cat + } + return c +} + +// errText renders err, or a fallback when nil. +func errText(err error) string { + if err == nil { + return "unavailable" + } + return err.Error() +} - return checks +// dirSecure reports whether dir exists, is a directory, and is not group/other +// writable — the integrity bar for the SQLite ledger and flock file. +func dirSecure(dir string) bool { + fi, err := os.Stat(dir) + if err != nil || !fi.IsDir() { + return false + } + return fi.Mode().Perm()&0o022 == 0 } -func renderChecks(cmd *cobra.Command, checks []docker.Check) { +func renderChecks(cmd *cobra.Command, checks []docker.Check, quiet bool) { w := cmd.OutOrStdout() for _, c := range checks { + if quiet && c.Status == docker.StatusOK { + continue + } var icon string switch c.Status { case docker.StatusOK: @@ -220,31 +585,56 @@ func renderChecks(cmd *cobra.Command, checks []docker.Check) { default: icon = "✗" } - fmt.Fprintf(w, "%s %-32s %s\n", icon, c.Name, c.Detail) + fixed := "" + if c.Fixed { + fixed = " (fixed)" + } + fmt.Fprintf(w, "%s %-32s %s%s\n", icon, c.Name, c.Detail, fixed) if c.Status != docker.StatusOK && c.Remediation != "" { fmt.Fprintf(w, " → %s\n", c.Remediation) } } } +func renderFixes(cmd *cobra.Command, fixes []fixResult) { + w := cmd.OutOrStdout() + fmt.Fprintln(w, "\n--fix: applying safe remediations…") + if len(fixes) == 0 { + fmt.Fprintln(w, " nothing to fix (no fixable check was failing)") + return + } + for _, f := range fixes { + if f.Fixed { + fmt.Fprintf(w, " ✓ %s: fixed — %s\n", f.ID, f.Detail) + continue + } + fmt.Fprintf(w, " ✗ %s: still failing — %s\n", f.ID, f.Detail) + if f.Remediation != "" { + fmt.Fprintf(w, " → %s\n", f.Remediation) + } + } +} + // fsCheck warns when dir is backed by a 9p/networked filesystem where SQLite and // flock locking are unreliable (spec 08). -func fsCheck(name, dir, remediation string) docker.Check { +func fsCheck(name, id, dir, remediation string) docker.Check { fsType := xdg.FilesystemType(dir) switch { case fsType == "": - return docker.Check{Name: name, Status: docker.StatusOK, Detail: dir} + return docker.Check{Name: name, ID: id, Category: catWarn, Status: docker.StatusOK, Detail: dir} case xdg.IsUnreliableLockFS(fsType): return docker.Check{ - Name: name, Status: docker.StatusWarn, + Name: name, ID: id, Category: catWarn, Status: docker.StatusWarn, Detail: fmt.Sprintf("%s is on %q where locking is unreliable", dir, fsType), Remediation: remediation, } default: - return docker.Check{Name: name, Status: docker.StatusOK, Detail: fmt.Sprintf("%s (%s)", dir, fsType)} + return docker.Check{Name: name, ID: id, Category: catWarn, Status: docker.StatusOK, Detail: fmt.Sprintf("%s (%s)", dir, fsType)} } } +// countFails counts only FAIL-status checks (warns never gate the exit code, per +// the spec 13 exit-code contract). func countFails(checks []docker.Check) int { n := 0 for _, c := range checks { diff --git a/internal/cli/doctor_fix_test.go b/internal/cli/doctor_fix_test.go new file mode 100644 index 0000000..4f81e19 --- /dev/null +++ b/internal/cli/doctor_fix_test.go @@ -0,0 +1,330 @@ +package cli + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/open-source-cloud/devstack/internal/docker" + "github.com/open-source-cloud/devstack/internal/generate" + "github.com/open-source-cloud/devstack/internal/state" +) + +// mkProbe builds a probe whose fix flips a live flag so the recheck can report a +// transition to OK. fixCalls counts fix invocations so we can assert a passing +// probe's fix is never run. +func mkProbe(id string, status docker.CheckStatus, fixable bool, fixCalls *int, fixErr error, fixed *bool) probe { + c := docker.Check{ID: id, Name: id, Status: status, Fixable: fixable} + if !fixable { + c.Remediation = "do it yourself" + return probe{check: c} + } + c.Remediation = "run --fix" + return probe{ + check: c, + fix: func(context.Context) error { + *fixCalls++ + if fixErr != nil { + return fixErr + } + *fixed = true + return nil + }, + recheck: func(context.Context) docker.Check { + if *fixed { + return docker.Check{ID: id, Name: id, Status: docker.StatusOK, Fixable: false, Detail: "repaired"} + } + return c + }, + } +} + +func TestApplyFixes(t *testing.T) { + t.Run("failing fixable check is fixed and re-probed green", func(t *testing.T) { + var calls int + var fixed bool + probes := []probe{mkProbe("net.shared", docker.StatusWarn, true, &calls, nil, &fixed)} + + res := applyFixes(context.Background(), probes) + + if calls != 1 { + t.Fatalf("fix should run exactly once, ran %d times", calls) + } + if len(res) != 1 || !res[0].Fixed { + t.Fatalf("expected one fixed result, got %+v", res) + } + // The probe is updated in place so the report reflects the post-fix state. + if probes[0].check.Status != docker.StatusOK || !probes[0].check.Fixed { + t.Fatalf("probe not updated post-fix: %+v", probes[0].check) + } + }) + + t.Run("non-fixable failing check is left with its remediation", func(t *testing.T) { + var calls int + var fixed bool + probes := []probe{mkProbe("trust.host", docker.StatusWarn, false, &calls, nil, &fixed)} + + res := applyFixes(context.Background(), probes) + + if calls != 0 { + t.Fatalf("a non-fixable probe must never be fixed, ran %d times", calls) + } + if len(res) != 0 { + t.Fatalf("non-fixable probe must not appear in fix results, got %+v", res) + } + if probes[0].check.Status != docker.StatusWarn || probes[0].check.Remediation == "" { + t.Fatalf("non-fixable probe should keep its warn + remediation: %+v", probes[0].check) + } + }) + + t.Run("passing check's fix is never invoked", func(t *testing.T) { + var calls int + var fixed bool + // Fixable but already OK — the fix must NOT run. + probes := []probe{mkProbe("fs.xdg", docker.StatusOK, true, &calls, nil, &fixed)} + + res := applyFixes(context.Background(), probes) + + if calls != 0 { + t.Fatalf("a passing check's fix must never run, ran %d times", calls) + } + if len(res) != 0 { + t.Fatalf("a passing check must not appear in fix results, got %+v", res) + } + }) + + t.Run("a failing fix reports still-failing with remediation", func(t *testing.T) { + var calls int + var fixed bool + probes := []probe{mkProbe("state.refs", docker.StatusFail, true, &calls, os.ErrPermission, &fixed)} + + res := applyFixes(context.Background(), probes) + + if calls != 1 { + t.Fatalf("fix should be attempted once, ran %d times", calls) + } + if len(res) != 1 || res[0].Fixed { + t.Fatalf("a failing fix must report not-fixed: %+v", res) + } + if res[0].Remediation == "" { + t.Fatalf("a failing fix must keep the remediation: %+v", res[0]) + } + // Status unchanged (recheck not applied on fix error). + if probes[0].check.Status != docker.StatusFail { + t.Fatalf("probe status should be unchanged after a failed fix: %+v", probes[0].check) + } + }) + + t.Run("mixed matrix: only the fixable non-OK probe is remediated", func(t *testing.T) { + var okCalls, warnCalls, plainCalls int + var f1, f2, f3 bool + probes := []probe{ + mkProbe("ok.fixable", docker.StatusOK, true, &okCalls, nil, &f1), // passing → skip + mkProbe("warn.fixable", docker.StatusWarn, true, &warnCalls, nil, &f2), // fixed + mkProbe("warn.plain", docker.StatusWarn, false, &plainCalls, nil, &f3), // non-fixable → skip + } + + res := applyFixes(context.Background(), probes) + + if okCalls != 0 || plainCalls != 0 { + t.Fatalf("only the fixable non-OK probe should run its fix: ok=%d plain=%d", okCalls, plainCalls) + } + if warnCalls != 1 { + t.Fatalf("the fixable warn probe should run once, ran %d", warnCalls) + } + if len(res) != 1 || res[0].ID != "warn.fixable" || !res[0].Fixed { + t.Fatalf("expected exactly the warn.fixable probe fixed, got %+v", res) + } + }) +} + +func TestDirSecure(t *testing.T) { + base := t.TempDir() + + tests := []struct { + name string + setup func() string + want bool + }{ + { + name: "missing dir is not secure", + setup: func() string { return filepath.Join(base, "nope") }, + want: false, + }, + { + name: "0700 dir is secure", + setup: func() string { + d := filepath.Join(base, "priv") + if err := os.Mkdir(d, 0o700); err != nil { + t.Fatal(err) + } + return d + }, + want: true, + }, + { + name: "group/world-writable dir is not secure", + setup: func() string { + d := filepath.Join(base, "loose") + if err := os.Mkdir(d, 0o777); err != nil { + t.Fatal(err) + } + if err := os.Chmod(d, 0o777); err != nil { + t.Fatal(err) + } + return d + }, + want: false, + }, + { + name: "a file (not a dir) is not secure", + setup: func() string { + f := filepath.Join(base, "afile") + if err := os.WriteFile(f, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + return f + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := dirSecure(tt.setup()); got != tt.want { + t.Fatalf("dirSecure = %v, want %v", got, tt.want) + } + }) + } +} + +// TestNetSharedProbeFixCreatesNetwork wires the real net.shared remediation: a +// missing external network warns fixable, the fix (EnsureNetwork under the lock) +// creates it, and the re-probe is green. +func TestNetSharedProbeFixCreatesNetwork(t *testing.T) { + mock := &docker.MockClient{Context: "ctx"} // no networks seeded + s := &doctorSession{ + client: mock, + lockPath: filepath.Join(t.TempDir(), "devstack.lock"), + } + ctx := context.Background() + + p := s.netSharedProbe(ctx) + if p.check.Status != docker.StatusWarn || !p.check.Fixable { + t.Fatalf("a missing network should be a fixable warn, got %+v", p.check) + } + + probes := []probe{p} + res := applyFixes(ctx, probes) + if len(res) != 1 || !res[0].Fixed { + t.Fatalf("network fix should report fixed, got %+v", res) + } + if !mock.Networks[generate.SharedNetwork] { + t.Fatalf("EnsureNetwork was not called: %+v", mock.Networks) + } + if probes[0].check.Status != docker.StatusOK { + t.Fatalf("re-probe should be OK after creating the network, got %+v", probes[0].check) + } +} + +// TestStateRefsProbeFixPrunesStaleRows wires the real state.refs remediation: a +// ledger ref row for a project with no live container is stale, and the fix +// (Reconcile under the flock) prunes it — never touching data. +func TestStateRefsProbeFixPrunesStaleRows(t *testing.T) { + db, err := state.Open(context.Background(), t.TempDir(), "ctx") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { db.Close() }) + // A ref row for a project that has NO live container → stale. + if err := db.AddRef("ghost", "web", "shared-postgres"); err != nil { + t.Fatal(err) + } + + mock := &docker.MockClient{Context: "ctx"} // no containers → ghost is stale + s := &doctorSession{ + client: mock, + db: db, + lockPath: filepath.Join(t.TempDir(), "devstack.lock"), + } + ctx := context.Background() + + p := s.stateRefsProbe(ctx) + if p.check.Status != docker.StatusWarn || !p.check.Fixable { + t.Fatalf("a stale ref row should be a fixable warn, got %+v", p.check) + } + + probes := []probe{p} + res := applyFixes(ctx, probes) + if len(res) != 1 || !res[0].Fixed { + t.Fatalf("ref prune should report fixed, got %+v", res) + } + refs, err := db.AllRefs() + if err != nil { + t.Fatal(err) + } + if len(refs) != 0 { + t.Fatalf("stale ref rows should be pruned, remaining: %+v", refs) + } +} + +// TestStateRefsProbeKeepsLiveRefs proves the reconcile never prunes a ref whose +// project IS live (no false-positive teardown of an in-use shared service). +func TestStateRefsProbeKeepsLiveRefs(t *testing.T) { + db, err := state.Open(context.Background(), t.TempDir(), "ctx") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { db.Close() }) + if err := db.AddRef("api", "db", "shared-postgres"); err != nil { + t.Fatal(err) + } + + mock := &docker.MockClient{ + Context: "ctx", + Containers: []docker.Container{{ + Name: "devstack-api-db-1", + State: "running", + Labels: map[string]string{generate.LabelManaged: "true", generate.LabelProject: "api"}, + }}, + } + s := &doctorSession{client: mock, db: db, lockPath: filepath.Join(t.TempDir(), "devstack.lock")} + + p := s.stateRefsProbe(context.Background()) + if p.check.Status != docker.StatusOK || p.check.Fixable { + t.Fatalf("a live ref should be OK and not fixable, got %+v", p.check) + } +} + +// TestXDGDirsProbeFixCreatesDirs exercises the real fs.xdg fix end to end: a +// missing XDG dir warns fixable, the fix creates it 0700, and the re-probe is +// green — all without touching any shared state. +func TestXDGDirsProbeFixCreatesDirs(t *testing.T) { + base := t.TempDir() + t.Setenv("XDG_DATA_HOME", filepath.Join(base, "data")) + t.Setenv("XDG_STATE_HOME", filepath.Join(base, "state")) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(base, "config")) + + s := &doctorSession{} + p := s.xdgDirsProbe() + if p.check.Status != docker.StatusWarn || !p.check.Fixable { + t.Fatalf("expected a fixable warn for missing XDG dirs, got %+v", p.check) + } + + probes := []probe{p} + res := applyFixes(context.Background(), probes) + if len(res) != 1 || !res[0].Fixed { + t.Fatalf("xdg dir fix should report fixed, got %+v", res) + } + if probes[0].check.Status != docker.StatusOK { + t.Fatalf("re-probe should be OK after creating dirs, got %+v", probes[0].check) + } + // The private data dir now exists with 0700. + fi, err := os.Stat(filepath.Join(base, "data", "devstack")) + if err != nil { + t.Fatalf("data dir not created: %v", err) + } + if fi.Mode().Perm() != 0o700 { + t.Fatalf("data dir perm = %o, want 0700", fi.Mode().Perm()) + } +} diff --git a/internal/docker/preflight.go b/internal/docker/preflight.go index 6741063..eb5096f 100644 --- a/internal/docker/preflight.go +++ b/internal/docker/preflight.go @@ -16,11 +16,21 @@ const ( // Check is one capability probe with a one-line remediation when not OK // (ARCHITECTURE §7.6: actionable errors are what close GitHub issues). +// +// ID/Category/Fixable are additive (spec 13): existing JSON consumers that only +// read name/status/detail/remediation keep working, while newer tooling can key +// off the stable ID, group by Category, and learn whether `doctor --fix` can +// remediate the probe. Fixed is set by `doctor --fix` when a remediation ran and +// the re-probe came back green. type Check struct { Name string `json:"name"` + ID string `json:"id,omitempty"` + Category string `json:"category,omitempty"` Status CheckStatus `json:"status"` Detail string `json:"detail"` Remediation string `json:"remediation,omitempty"` + Fixable bool `json:"fixable"` + Fixed bool `json:"fixed,omitempty"` } // Preflight probes the external tools devstack drives. The daemon probe is