diff --git a/src/cmd/doctor.go b/src/cmd/doctor.go new file mode 100644 index 0000000..cc0a05e --- /dev/null +++ b/src/cmd/doctor.go @@ -0,0 +1,190 @@ +package cmd + +import ( + "bufio" + "fmt" + "os" + "strings" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/doctor" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/ui" + "github.com/spf13/cobra" +) + +var ( + doctorFix bool + doctorYes bool + doctorNoFix bool +) + +var doctorCmd = &cobra.Command{ + Use: "doctor", + Short: "Diagnose dtvem configuration issues", + Long: `Run a battery of checks against your dtvem installation and surface any +configuration problems it finds. + +By default, doctor is read-only: it prints a report and exits non-zero +when an error-severity finding is present, but it does not modify any +state. Pass --fix to enable interactive remediation; pair with --yes +to apply every fixable finding without prompting. + +Findings are grouped by severity. Each one is marked either [fixable] +(doctor knows how to remediate it) or [manual] (you'll see step-by-step +instructions). Manual findings are never auto-applied, even with --fix. + +Examples: + dtvem doctor # Report problems, don't change anything + dtvem doctor --fix # Prompt to fix each fixable finding + dtvem doctor --fix --yes # Apply every fixable finding non-interactively + dtvem doctor --no-fix # Explicit read-only mode (for scripts)`, + Run: func(cmd *cobra.Command, args []string) { + if doctorFix && doctorNoFix { + ui.Error("--fix and --no-fix are mutually exclusive") + os.Exit(2) + } + + result := doctor.RunAll() + renderReport(result) + + // Apply fixes if requested. We run this even when there are no + // error-severity findings — warnings can also be fixable, and + // nothing prevents the user from cleaning those up too. + if doctorFix { + applyFixes(result, doctorYes) + } + + if result.HasErrors() { + // Non-zero exit so CI / wrapping scripts can react. We use + // os.Exit rather than returning an error from Run so we + // don't trigger Cobra's "Error:" prefix on the rendered + // report. + os.Exit(1) + } + }, +} + +// renderReport prints findings grouped into a passing section and a +// problems section. We deliberately keep the layout close to the +// example in the GitHub issue so the report is greppable and the user +// can find specific finding shapes by eye. +func renderReport(r doctor.Result) { + ui.Header("dtvem doctor") + fmt.Println() + + var ok, problems []doctor.CheckResult + for _, cr := range r.Results { + if cr.Finding.OK { + ok = append(ok, cr) + } else { + problems = append(problems, cr) + } + } + + for _, cr := range problems { + printFinding(cr.Finding) + } + + for _, cr := range ok { + ui.Success("%s", cr.Finding.Title) + } + + fmt.Println() + summarize(len(ok), len(problems), r.HasErrors()) +} + +// printFinding renders a single non-OK finding: a severity-colored +// title line with [fixable] or [manual] tag, the aligned details +// block, and the resolution text. +func printFinding(f doctor.Finding) { + tag := "[manual]" + if f.Fixable() { + tag = "[fixable]" + } + + header := fmt.Sprintf("%s %s", f.Title, tag) + switch f.Severity { + case doctor.SeverityError: + ui.Error("%s", header) + case doctor.SeverityWarning: + ui.Warning("%s", header) + default: + ui.Info("%s", header) + } + + // Align the keys so the values form a tidy column. The longest key + // drives column width; we don't pad past it. + maxKey := 0 + for _, d := range f.Details { + if l := len(d.Key); l > maxKey { + maxKey = l + } + } + for _, d := range f.Details { + pad := strings.Repeat(" ", maxKey-len(d.Key)) + fmt.Printf(" %s:%s %s\n", d.Key, pad, d.Value) + } + + if f.Resolution != "" { + // Indent every line of the resolution so multi-line manual + // instructions stay visually grouped under the finding. + for _, line := range strings.Split(f.Resolution, "\n") { + fmt.Printf(" %s\n", line) + } + } + fmt.Println() +} + +// summarize prints the closing one-liner so users (and CI logs) see a +// quick result without having to count findings themselves. +func summarize(ok, problems int, hasErrors bool) { + if problems == 0 { + ui.Success("All %d check(s) passed", ok) + return + } + if hasErrors { + ui.Error("%d problem(s) found across %d check(s)", problems, ok+problems) + } else { + ui.Warning("%d non-error problem(s) found across %d check(s)", problems, ok+problems) + } +} + +// applyFixes walks the fixable findings and applies each one — either +// after a y/N prompt, or immediately when --yes was passed. We print a +// running tally so users see what doctor did, in the order it did it. +func applyFixes(r doctor.Result, yes bool) { + fixable := r.Fixable() + if len(fixable) == 0 { + ui.Info("No fixable findings to apply.") + return + } + + fmt.Println() + ui.Header("Applying fixes") + fmt.Println() + + reader := bufio.NewReader(os.Stdin) + for _, cr := range fixable { + if !yes { + fmt.Printf("Fix: %s? [y/N] ", cr.Finding.Title) + line, _ := reader.ReadString('\n') + line = strings.ToLower(strings.TrimSpace(line)) + if line != "y" && line != "yes" { + ui.Info("Skipped: %s", cr.Finding.Title) + continue + } + } + + if err := cr.Finding.Fix(); err != nil { + ui.Error("Fix failed for %s: %v", cr.Finding.Title, err) + continue + } + ui.Success("Fixed: %s", cr.Finding.Title) + } +} + +func init() { + doctorCmd.Flags().BoolVar(&doctorFix, "fix", false, "Interactively apply fixes for fixable findings") + doctorCmd.Flags().BoolVarP(&doctorYes, "yes", "y", false, "Skip prompts when --fix is set; apply all fixable findings") + doctorCmd.Flags().BoolVar(&doctorNoFix, "no-fix", false, "Explicit read-only mode (for scripts)") + rootCmd.AddCommand(doctorCmd) +} diff --git a/src/internal/doctor/check_configured_runtimes.go b/src/internal/doctor/check_configured_runtimes.go new file mode 100644 index 0000000..ad23cd0 --- /dev/null +++ b/src/internal/doctor/check_configured_runtimes.go @@ -0,0 +1,140 @@ +package doctor + +import ( + "fmt" + "os" + "sort" + "strings" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/config" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/runtime" +) + +// configuredRuntimesCheck verifies that every runtime/version pair in +// the global ~/.dtvem/config/runtimes.json points at an installed +// version. A mismatch means `dtvem current` will report a version +// dtvem can't actually execute, and shim invocations for that runtime +// will fail with a confusing "version not installed" error rather +// than a useful one at config-load time. +// +// The fix isn't automatable in general — doctor can't tell whether +// the user wants to install the configured version or edit the +// config to match what's installed — so this is a manual check with +// a one-line `dtvem install` suggestion per mismatch. +type configuredRuntimesCheck struct { + configPath func() string + readConfig func(path string) (config.RuntimesConfig, error) + getProvider func(name string) (runtime.ShimProvider, error) +} + +func newConfiguredRuntimesCheck() *configuredRuntimesCheck { + return &configuredRuntimesCheck{ + configPath: config.GlobalConfigPath, + readConfig: config.ReadAllRuntimes, + getProvider: runtime.GetShimProvider, + } +} + +func (configuredRuntimesCheck) Name() string { return "configured-runtimes-installed" } + +func (c configuredRuntimesCheck) Run() Finding { + cfgPath := c.configPath() + cfg, err := c.readConfig(cfgPath) + if err != nil { + if os.IsNotExist(err) { + return Finding{OK: true, Title: "No global runtimes config to check"} + } + return Finding{ + Severity: SeverityWarning, + Title: "Could not read global runtimes config", + Details: []Detail{{Key: "Path", Value: cfgPath}, {Key: "Error", Value: err.Error()}}, + Resolution: "Check that " + cfgPath + " is valid JSON and readable.", + } + } + if len(cfg) == 0 { + return Finding{OK: true, Title: "Global runtimes config is empty"} + } + + type problem struct { + runtimeName string + version string + displayName string + detail string + } + var problems []problem + + for name, version := range cfg { + p, err := c.getProvider(name) + if err != nil { + problems = append(problems, problem{ + runtimeName: name, + version: version, + displayName: name, + detail: "configured runtime is unknown to dtvem (no provider registered)", + }) + continue + } + + installed, err := p.IsInstalled(version) + if err != nil { + problems = append(problems, problem{ + runtimeName: name, + version: version, + displayName: p.DisplayName(), + detail: fmt.Sprintf("could not check install status: %v", err), + }) + continue + } + if !installed { + problems = append(problems, problem{ + runtimeName: name, + version: version, + displayName: p.DisplayName(), + detail: fmt.Sprintf("version %s is not installed (run `dtvem install %s %s`)", version, name, version), + }) + } + } + + if len(problems) == 0 { + return Finding{OK: true, Title: "All configured runtimes are installed"} + } + + // Stable order so the report doesn't shuffle between runs. + sort.Slice(problems, func(i, j int) bool { + if problems[i].runtimeName == problems[j].runtimeName { + return problems[i].version < problems[j].version + } + return problems[i].runtimeName < problems[j].runtimeName + }) + + details := make([]Detail, 0, len(problems)+1) + details = append(details, Detail{Key: "Config", Value: cfgPath}) + for _, p := range problems { + details = append(details, Detail{Key: p.displayName, Value: p.detail}) + } + + return Finding{ + Severity: SeverityError, + Title: fmt.Sprintf("%d configured runtime version%s not installed", len(problems), plural(len(problems), "", "s")), + Details: details, + Resolution: configuredRuntimesResolution(problems[0].runtimeName, problems[0].version), + } +} + +// configuredRuntimesResolution suggests the install command for the +// first problem so the user has a concrete next step. Listing every +// install command in the resolution would duplicate the detail block +// without adding info. +func configuredRuntimesResolution(runtimeName, version string) string { + return strings.Join([]string{ + "Install the missing version(s) listed above, or edit", + " " + config.GlobalConfigPath(), + "to reference versions that are installed.", + "", + fmt.Sprintf("Example: dtvem install %s %s", runtimeName, version), + }, "\n") +} + +func init() { + Register(newConfiguredRuntimesCheck()) +} diff --git a/src/internal/doctor/check_configured_runtimes_test.go b/src/internal/doctor/check_configured_runtimes_test.go new file mode 100644 index 0000000..65ebedd --- /dev/null +++ b/src/internal/doctor/check_configured_runtimes_test.go @@ -0,0 +1,188 @@ +package doctor + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/config" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/runtime" +) + +// installAwareProvider is a fakeProvider variant that tracks +// IsInstalled answers per (name, version) pair. The runtime registry +// can't be cleanly populated in tests, so we route through the +// check's injected getProvider closure instead. +type installAwareProvider struct { + *fakeProvider + installed map[string]bool + checkErr error +} + +func (p *installAwareProvider) IsInstalled(version string) (bool, error) { + if p.checkErr != nil { + return false, p.checkErr + } + return p.installed[version], nil +} + +// configuredCheckWith returns a check wired with a synthetic config +// and provider lookup. Providers are keyed by runtime name; missing +// keys fall through to "unknown runtime" handling. +func configuredCheckWith(cfg config.RuntimesConfig, providers map[string]*installAwareProvider, cfgErr error) *configuredRuntimesCheck { + c := newConfiguredRuntimesCheck() + c.configPath = func() string { return "/tmp/runtimes.json" } + c.readConfig = func(_ string) (config.RuntimesConfig, error) { return cfg, cfgErr } + c.getProvider = func(name string) (runtime.ShimProvider, error) { + p, ok := providers[name] + if !ok { + return nil, errors.New("provider not found: " + name) + } + return p, nil + } + return c +} + +func newInstallAwareProvider(name, display string, installed map[string]bool) *installAwareProvider { + return &installAwareProvider{ + fakeProvider: &fakeProvider{name: name, displayName: display, shims: []string{name}}, + installed: installed, + } +} + +func TestConfiguredRuntimesCheck_AllInstalledIsOK(t *testing.T) { + cfg := config.RuntimesConfig{"python": "3.11.0", "node": "22.0.0"} + providers := map[string]*installAwareProvider{ + "python": newInstallAwareProvider("python", "Python", map[string]bool{"3.11.0": true}), + "node": newInstallAwareProvider("node", "Node.js", map[string]bool{"22.0.0": true}), + } + got := configuredCheckWith(cfg, providers, nil).Run() + if !got.OK { + t.Errorf("expected OK when all configured versions are installed, got %#v", got) + } +} + +func TestConfiguredRuntimesCheck_FlagsMissingInstall(t *testing.T) { + cfg := config.RuntimesConfig{"python": "3.11.0"} + providers := map[string]*installAwareProvider{ + "python": newInstallAwareProvider("python", "Python", map[string]bool{}), + } + got := configuredCheckWith(cfg, providers, nil).Run() + if got.OK { + t.Fatalf("expected non-OK when configured version is missing, got OK") + } + if got.Severity != SeverityError { + t.Errorf("severity: got %s, want error", got.Severity) + } + if got.Fixable() { + t.Errorf("configured-runtimes check should be manual") + } + + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, "3.11.0") { + t.Errorf("expected version 3.11.0 in details, got %#v", got.Details) + } + if !strings.Contains(got.Resolution, "dtvem install") { + t.Errorf("resolution should suggest `dtvem install`, got %q", got.Resolution) + } +} + +func TestConfiguredRuntimesCheck_UnknownRuntimeIsFlagged(t *testing.T) { + cfg := config.RuntimesConfig{"madeup": "1.2.3"} + got := configuredCheckWith(cfg, map[string]*installAwareProvider{}, nil).Run() + if got.OK { + t.Fatalf("expected non-OK when config references unknown runtime, got OK") + } + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, "unknown") { + t.Errorf("expected detail mentioning 'unknown', got %#v", got.Details) + } +} + +func TestConfiguredRuntimesCheck_IsInstalledErrorIsFlagged(t *testing.T) { + cfg := config.RuntimesConfig{"python": "3.11.0"} + failing := newInstallAwareProvider("python", "Python", nil) + failing.checkErr = errors.New("disk read failed") + providers := map[string]*installAwareProvider{"python": failing} + + got := configuredCheckWith(cfg, providers, nil).Run() + if got.OK { + t.Fatalf("expected non-OK when IsInstalled errors, got OK") + } + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, "disk read failed") { + t.Errorf("expected error text in detail, got %#v", got.Details) + } +} + +func TestConfiguredRuntimesCheck_MissingConfigFileIsOK(t *testing.T) { + missing := &os.PathError{Op: "open", Path: "/tmp/runtimes.json", Err: os.ErrNotExist} + got := configuredCheckWith(nil, nil, missing).Run() + if !got.OK { + t.Errorf("expected OK when config file is missing, got %#v", got) + } +} + +func TestConfiguredRuntimesCheck_UnreadableConfigIsWarning(t *testing.T) { + got := configuredCheckWith(nil, nil, errors.New("corrupt json")).Run() + if got.OK { + t.Fatalf("expected non-OK when config can't be read, got OK") + } + if got.Severity != SeverityWarning { + t.Errorf("severity: got %s, want warning", got.Severity) + } +} + +func TestConfiguredRuntimesCheck_EmptyConfigIsOK(t *testing.T) { + got := configuredCheckWith(config.RuntimesConfig{}, nil, nil).Run() + if !got.OK { + t.Errorf("expected OK when config is empty, got %#v", got) + } +} + +func TestConfiguredRuntimesCheck_DetailsAreSorted(t *testing.T) { + cfg := config.RuntimesConfig{ + "zruntime": "1.0.0", + "aruntime": "1.0.0", + } + providers := map[string]*installAwareProvider{ + "zruntime": newInstallAwareProvider("zruntime", "Z", map[string]bool{}), + "aruntime": newInstallAwareProvider("aruntime", "A", map[string]bool{}), + } + got := configuredCheckWith(cfg, providers, nil).Run() + + // Details start with "Config" then the per-runtime entries + // sorted by name. Confirm "A" precedes "Z". + idxA, idxZ := -1, -1 + for i, d := range got.Details { + if d.Key == "A" { + idxA = i + } + if d.Key == "Z" { + idxZ = i + } + } + if idxA < 0 || idxZ < 0 || idxA > idxZ { + t.Errorf("expected A detail before Z detail; details: %#v", got.Details) + } +} + +func TestConfiguredRuntimesCheck_Registered(t *testing.T) { + found := false + for _, c := range All() { + if c.Name() == "configured-runtimes-installed" { + found = true + break + } + } + if !found { + t.Errorf("configured-runtimes-installed check is not in the default registry") + } +} + +// Touch a config file constant just to make sure config import works +// in this test file's package — keeps lints happy when test bodies +// don't otherwise reference imported packages. +var _ = filepath.Join diff --git a/src/internal/doctor/check_empty_shims.go b/src/internal/doctor/check_empty_shims.go new file mode 100644 index 0000000..dc837c8 --- /dev/null +++ b/src/internal/doctor/check_empty_shims.go @@ -0,0 +1,168 @@ +package doctor + +import ( + "fmt" + "os" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/config" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/shim" +) + +// emptyShimsCheck looks for the specific broken state where the user +// has installed runtimes (so versions/ contains at least one runtime +// directory with at least one version) but the shims/ directory is +// empty or missing. In that state, `python --version` from the shell +// will route to the system runtime — or fail outright — even though +// dtvem thinks it owns the command. +// +// The fix is to run `dtvem reshim`, which is safe and idempotent: it +// recreates shims from the installed versions on disk without touching +// PATH or anything outside ~/.dtvem. +type emptyShimsCheck struct { + // versionsDir, shimsDir, and newManager are injected so tests can + // drive the check against a synthetic install layout without + // touching DTVEM_ROOT (and so the fix path can be exercised + // without producing a real dtvem-shim binary). + versionsDir func() string + shimsDir func() string + newManager func() (rehasher, error) +} + +// rehasher is the subset of shim.Manager used by emptyShimsCheck.Fix. +// Defining it locally keeps the check's tests from having to build a +// full shim.Manager (which requires a real dtvem-shim binary on disk). +type rehasher interface { + Rehash() (*shim.RehashResult, error) +} + +func newEmptyShimsCheck() *emptyShimsCheck { + return &emptyShimsCheck{ + versionsDir: func() string { return config.DefaultPaths().Versions }, + shimsDir: func() string { return config.DefaultPaths().Shims }, + newManager: func() (rehasher, error) { + m, err := shim.NewManager() + if err != nil { + return nil, err + } + return m, nil + }, + } +} + +func (emptyShimsCheck) Name() string { return "empty-shims-directory" } + +func (c emptyShimsCheck) Run() Finding { + versionsCount, err := countInstalledRuntimeVersions(c.versionsDir()) + if err != nil { + return Finding{ + Severity: SeverityWarning, + Title: "Could not inspect installed runtimes", + Details: []Detail{{Key: "Error", Value: err.Error()}}, + Resolution: "Check that " + c.versionsDir() + " is accessible.", + } + } + + // No installed runtimes means an empty shims/ is expected — not a + // problem to surface to the user. + if versionsCount == 0 { + return Finding{OK: true, Title: "No installed runtimes (no shims expected)"} + } + + shimsCount, err := countShimFiles(c.shimsDir()) + if err != nil { + return Finding{ + Severity: SeverityWarning, + Title: "Could not inspect shims directory", + Details: []Detail{{Key: "Error", Value: err.Error()}}, + Resolution: "Check that " + c.shimsDir() + " is accessible.", + } + } + + if shimsCount > 0 { + return Finding{OK: true, Title: "Shims directory matches installed runtimes"} + } + + return Finding{ + Severity: SeverityError, + Title: "Shims directory is empty but runtimes are installed", + Details: []Detail{ + {Key: "Versions found", Value: fmt.Sprintf("%d", versionsCount)}, + {Key: "Shims found", Value: "0"}, + {Key: "Shims dir", Value: c.shimsDir()}, + }, + Resolution: "Run `dtvem reshim` to recreate the shim files.", + Fix: func() error { + m, err := c.newManager() + if err != nil { + return fmt.Errorf("could not create shim manager: %w", err) + } + if _, err := m.Rehash(); err != nil { + return fmt.Errorf("reshim failed: %w", err) + } + return nil + }, + } +} + +// countInstalledRuntimeVersions returns the total number of runtime +// versions installed under versionsDir. A runtime with a directory but +// no version subdirectory contributes zero. +// +// Returns (0, nil) when versionsDir doesn't exist — that's the +// "freshly initialized but no installs yet" case, which is normal and +// the caller treats as "no problem". +func countInstalledRuntimeVersions(versionsDir string) (int, error) { + entries, err := os.ReadDir(versionsDir) + if err != nil { + if os.IsNotExist(err) { + return 0, nil + } + return 0, err + } + + total := 0 + for _, e := range entries { + if !e.IsDir() { + continue + } + runtimeDir := versionsDir + string(os.PathSeparator) + e.Name() + versionEntries, err := os.ReadDir(runtimeDir) + if err != nil { + continue + } + for _, v := range versionEntries { + if v.IsDir() { + total++ + } + } + } + return total, nil +} + +// countShimFiles returns the number of non-directory entries in +// shimsDir. We don't try to distinguish .exe from .cmd wrappers on +// Windows — any file at all is enough to conclude the directory has +// been populated. +// +// Returns (0, nil) for a missing directory: that's the most common +// shape of the "empty shims" failure we're detecting. +func countShimFiles(shimsDir string) (int, error) { + entries, err := os.ReadDir(shimsDir) + if err != nil { + if os.IsNotExist(err) { + return 0, nil + } + return 0, err + } + count := 0 + for _, e := range entries { + if !e.IsDir() { + count++ + } + } + return count, nil +} + +func init() { + Register(newEmptyShimsCheck()) +} diff --git a/src/internal/doctor/check_empty_shims_test.go b/src/internal/doctor/check_empty_shims_test.go new file mode 100644 index 0000000..0c11600 --- /dev/null +++ b/src/internal/doctor/check_empty_shims_test.go @@ -0,0 +1,169 @@ +package doctor + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/shim" +) + +// fakeRehasher is a rehasher impl used to verify the Fix closure wires +// through to Rehash without standing up a real shim.Manager (which +// needs the dtvem-shim binary on disk). +type fakeRehasher struct { + called bool + err error +} + +func (f *fakeRehasher) Rehash() (*shim.RehashResult, error) { + f.called = true + if f.err != nil { + return nil, f.err + } + return &shim.RehashResult{TotalShims: 1}, nil +} + +// emptyShimsLayout produces a fresh versions/shims directory pair +// rooted at root and seeded with the requested counts. Each call to +// versionsDir() returns versions, and shimsDir() returns shims, so the +// check can be exercised against a clean filesystem state per test. +func emptyShimsLayout(t *testing.T, numRuntimes, numVersions, numShims int) (versions, shims string) { + t.Helper() + root := t.TempDir() + versions = filepath.Join(root, "versions") + shims = filepath.Join(root, "shims") + + if numRuntimes > 0 { + if err := os.MkdirAll(versions, 0755); err != nil { + t.Fatalf("mkdir versions: %v", err) + } + for i := range numRuntimes { + runtimeDir := filepath.Join(versions, "runtime"+string(rune('a'+i))) + if err := os.MkdirAll(runtimeDir, 0755); err != nil { + t.Fatalf("mkdir runtime: %v", err) + } + for j := range numVersions { + if err := os.MkdirAll(filepath.Join(runtimeDir, "v"+string(rune('0'+j))), 0755); err != nil { + t.Fatalf("mkdir version: %v", err) + } + } + } + } + + if numShims > 0 { + if err := os.MkdirAll(shims, 0755); err != nil { + t.Fatalf("mkdir shims: %v", err) + } + for i := range numShims { + f := filepath.Join(shims, "shim"+string(rune('a'+i))) + if err := os.WriteFile(f, []byte{}, 0644); err != nil { + t.Fatalf("write shim: %v", err) + } + } + } + + return versions, shims +} + +func newEmptyShimsCheckFor(versions, shims string, rh *fakeRehasher) *emptyShimsCheck { + c := newEmptyShimsCheck() + c.versionsDir = func() string { return versions } + c.shimsDir = func() string { return shims } + c.newManager = func() (rehasher, error) { return rh, nil } + return c +} + +func TestEmptyShimsCheck_NoRuntimesIsOK(t *testing.T) { + // No runtimes installed → empty shims is expected, not a problem. + versions, shims := emptyShimsLayout(t, 0, 0, 0) + got := newEmptyShimsCheckFor(versions, shims, nil).Run() + if !got.OK { + t.Errorf("expected OK with no runtimes installed, got %#v", got) + } +} + +func TestEmptyShimsCheck_RuntimesAndShimsBothPresentIsOK(t *testing.T) { + versions, shims := emptyShimsLayout(t, 1, 1, 3) + got := newEmptyShimsCheckFor(versions, shims, nil).Run() + if !got.OK { + t.Errorf("expected OK with runtimes and shims present, got %#v", got) + } +} + +func TestEmptyShimsCheck_RuntimesPresentButShimsEmpty(t *testing.T) { + versions, shims := emptyShimsLayout(t, 1, 1, 0) + got := newEmptyShimsCheckFor(versions, shims, &fakeRehasher{}).Run() + if got.OK { + t.Fatalf("expected non-OK when runtimes exist but shims dir is empty, got OK") + } + if got.Severity != SeverityError { + t.Errorf("severity: got %s, want error", got.Severity) + } + if !got.Fixable() { + t.Errorf("empty-shims check should be fixable") + } +} + +func TestEmptyShimsCheck_FixInvokesRehash(t *testing.T) { + versions, shims := emptyShimsLayout(t, 1, 1, 0) + rh := &fakeRehasher{} + got := newEmptyShimsCheckFor(versions, shims, rh).Run() + if !got.Fixable() { + t.Fatalf("precondition: expected fixable finding") + } + if err := got.Fix(); err != nil { + t.Fatalf("Fix returned error: %v", err) + } + if !rh.called { + t.Errorf("Fix did not invoke Rehash") + } +} + +func TestEmptyShimsCheck_FixPropagatesRehashError(t *testing.T) { + versions, shims := emptyShimsLayout(t, 1, 1, 0) + wantErr := errors.New("disk full") + rh := &fakeRehasher{err: wantErr} + got := newEmptyShimsCheckFor(versions, shims, rh).Run() + err := got.Fix() + if err == nil { + t.Fatal("expected Fix to return an error") + } + if !errors.Is(err, wantErr) { + t.Errorf("Fix error: got %v, want chain containing %v", err, wantErr) + } +} + +func TestEmptyShimsCheck_RuntimeDirWithNoVersionsCountsAsZero(t *testing.T) { + // A runtime directory with no version subdirectories means no + // actual installs — even if there are several runtime folders, the + // check should treat this as "nothing installed" rather than + // flagging missing shims. + versions, shims := emptyShimsLayout(t, 2, 0, 0) + got := newEmptyShimsCheckFor(versions, shims, nil).Run() + if !got.OK { + t.Errorf("expected OK when no version subdirs exist, got %#v", got) + } +} + +func TestEmptyShimsCheck_MissingVersionsDirIsOK(t *testing.T) { + // versions/ not existing yet is the freshly-initialized state. + got := newEmptyShimsCheckFor("/definitely/does/not/exist/versions", "/definitely/does/not/exist/shims", nil).Run() + if !got.OK { + t.Errorf("expected OK when versions dir missing, got %#v", got) + } +} + +func TestEmptyShimsCheck_Registered(t *testing.T) { + found := false + for _, c := range All() { + if c.Name() == "empty-shims-directory" { + found = true + break + } + } + if !found { + t.Errorf("empty-shims-directory check is not in the default registry") + } +} diff --git a/src/internal/doctor/check_runtime_executable.go b/src/internal/doctor/check_runtime_executable.go new file mode 100644 index 0000000..4ec5e96 --- /dev/null +++ b/src/internal/doctor/check_runtime_executable.go @@ -0,0 +1,197 @@ +package doctor + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/config" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/runtime" +) + +// runtimeExecutableCheck walks ~/.dtvem/versions and, for every +// installed runtime version, asks the provider where the executable +// should live and confirms it's actually there. Missing executables +// most commonly result from interrupted installs (the version +// directory was created but the download/extract didn't finish) or +// from manual edits to the versions tree. +// +// The fix isn't safe to automate. Reinstalling the runtime version +// is what the user wants, but doctor doing that without prompting +// could overwrite an incomplete install the user was intentionally +// pinning for forensics. We surface a one-line install command per +// missing version so remediation is one copy-paste away. +type runtimeExecutableCheck struct { + versionsDir func() string + getProvider func(name string) (runtime.ShimProvider, error) +} + +func newRuntimeExecutableCheck() *runtimeExecutableCheck { + return &runtimeExecutableCheck{ + versionsDir: func() string { return config.DefaultPaths().Versions }, + getProvider: runtime.GetShimProvider, + } +} + +func (runtimeExecutableCheck) Name() string { return "runtime-executable-present" } + +func (c runtimeExecutableCheck) Run() Finding { + root := c.versionsDir() + installs, err := listInstalledVersions(root) + if err != nil { + return Finding{ + Severity: SeverityWarning, + Title: "Could not list installed runtime versions", + Details: []Detail{{Key: "Error", Value: err.Error()}}, + Resolution: "Check that " + root + " is readable.", + } + } + if len(installs) == 0 { + return Finding{OK: true, Title: "No installed runtime versions to check"} + } + + type problem struct { + displayName string + runtimeName string + version string + detail string + } + var problems []problem + + for _, inst := range installs { + p, err := c.getProvider(inst.runtimeName) + if err != nil { + problems = append(problems, problem{ + displayName: inst.runtimeName, + runtimeName: inst.runtimeName, + version: inst.version, + detail: "no provider registered for this runtime — orphaned data?", + }) + continue + } + + execPath, err := p.ExecutablePath(inst.version) + if err != nil { + problems = append(problems, problem{ + displayName: p.DisplayName(), + runtimeName: inst.runtimeName, + version: inst.version, + detail: fmt.Sprintf("provider could not resolve executable path: %v", err), + }) + continue + } + if execPath == "" { + problems = append(problems, problem{ + displayName: p.DisplayName(), + runtimeName: inst.runtimeName, + version: inst.version, + detail: "provider returned an empty executable path", + }) + continue + } + info, err := os.Stat(execPath) + if err != nil { + problems = append(problems, problem{ + displayName: p.DisplayName(), + runtimeName: inst.runtimeName, + version: inst.version, + detail: fmt.Sprintf("expected at %s — %v", execPath, err), + }) + continue + } + if info.IsDir() { + problems = append(problems, problem{ + displayName: p.DisplayName(), + runtimeName: inst.runtimeName, + version: inst.version, + detail: fmt.Sprintf("expected file but found directory at %s", execPath), + }) + } + } + + if len(problems) == 0 { + return Finding{OK: true, Title: "All installed runtime versions have their executable"} + } + + // Stable ordering so the report is deterministic. + sort.Slice(problems, func(i, j int) bool { + if problems[i].runtimeName == problems[j].runtimeName { + return problems[i].version < problems[j].version + } + return problems[i].runtimeName < problems[j].runtimeName + }) + + details := make([]Detail, 0, len(problems)) + for _, p := range problems { + details = append(details, Detail{ + Key: fmt.Sprintf("%s %s", p.displayName, p.version), + Value: p.detail, + }) + } + + return Finding{ + Severity: SeverityError, + Title: fmt.Sprintf("%d installed runtime version%s missing its executable", len(problems), plural(len(problems), "", "s")), + Details: details, + Resolution: strings.Join([]string{ + "Reinstall the affected version(s) to restore the executable. Example:", + fmt.Sprintf(" dtvem uninstall %s %s && dtvem install %s %s", + problems[0].runtimeName, problems[0].version, + problems[0].runtimeName, problems[0].version), + }, "\n"), + } +} + +// installedVersion is the on-disk (runtime, version) pair we encounter +// while walking ~/.dtvem/versions. Defined inline because it's only +// used by this check and exporting it would invite reuse elsewhere +// against an interface that isn't ours to stabilize yet. +type installedVersion struct { + runtimeName string + version string +} + +// listInstalledVersions returns every (runtime, version) pair found +// under versionsDir, sorted by runtime then version for stable output. +// A missing versionsDir returns (nil, nil) — that's the "no installs +// yet" state, not an error condition. +func listInstalledVersions(versionsDir string) ([]installedVersion, error) { + runtimeEntries, err := os.ReadDir(versionsDir) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, err + } + + var out []installedVersion + for _, r := range runtimeEntries { + if !r.IsDir() { + continue + } + versionEntries, err := os.ReadDir(filepath.Join(versionsDir, r.Name())) + if err != nil { + continue + } + for _, v := range versionEntries { + if !v.IsDir() { + continue + } + out = append(out, installedVersion{runtimeName: r.Name(), version: v.Name()}) + } + } + + sort.Slice(out, func(i, j int) bool { + if out[i].runtimeName == out[j].runtimeName { + return out[i].version < out[j].version + } + return out[i].runtimeName < out[j].runtimeName + }) + return out, nil +} + +func init() { + Register(newRuntimeExecutableCheck()) +} diff --git a/src/internal/doctor/check_runtime_executable_test.go b/src/internal/doctor/check_runtime_executable_test.go new file mode 100644 index 0000000..f6319ff --- /dev/null +++ b/src/internal/doctor/check_runtime_executable_test.go @@ -0,0 +1,213 @@ +package doctor + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/runtime" +) + +// execAwareProvider extends fakeProvider with a configurable +// ExecutablePath result so tests can plant working and broken +// installs side by side. +type execAwareProvider struct { + *fakeProvider + execPaths map[string]string + execErr error +} + +func (p *execAwareProvider) ExecutablePath(version string) (string, error) { + if p.execErr != nil { + return "", p.execErr + } + return p.execPaths[version], nil +} + +// installVersionDir creates versions/// under root +// and returns the runtime directory's full path so the test can plant +// an executable inside it. +func installVersionDir(t *testing.T, root, runtime, version string) string { + t.Helper() + dir := filepath.Join(root, "versions", runtime, version) + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } + return dir +} + +func runtimeExecCheckWith(versionsDir string, providers map[string]*execAwareProvider) *runtimeExecutableCheck { + c := newRuntimeExecutableCheck() + c.versionsDir = func() string { return versionsDir } + c.getProvider = func(name string) (runtime.ShimProvider, error) { + p, ok := providers[name] + if !ok { + return nil, errors.New("provider not registered: " + name) + } + return p, nil + } + return c +} + +func TestRuntimeExecutableCheck_NoInstallsIsOK(t *testing.T) { + root := t.TempDir() + got := runtimeExecCheckWith(filepath.Join(root, "versions"), nil).Run() + if !got.OK { + t.Errorf("expected OK with no installs, got %#v", got) + } +} + +func TestRuntimeExecutableCheck_HealthyInstallIsOK(t *testing.T) { + root := t.TempDir() + versionDir := installVersionDir(t, root, "python", "3.11.0") + execPath := filepath.Join(versionDir, "bin", "python") + if err := os.MkdirAll(filepath.Dir(execPath), 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(execPath, []byte("stub"), 0755); err != nil { + t.Fatalf("write: %v", err) + } + + providers := map[string]*execAwareProvider{ + "python": { + fakeProvider: &fakeProvider{name: "python", displayName: "Python", shims: []string{"python"}}, + execPaths: map[string]string{"3.11.0": execPath}, + }, + } + got := runtimeExecCheckWith(filepath.Join(root, "versions"), providers).Run() + if !got.OK { + t.Errorf("expected OK with healthy install, got %#v", got) + } +} + +func TestRuntimeExecutableCheck_MissingExecutableIsError(t *testing.T) { + root := t.TempDir() + installVersionDir(t, root, "python", "3.11.0") + // No file at execPath — install is incomplete. + execPath := filepath.Join(root, "versions", "python", "3.11.0", "bin", "python") + + providers := map[string]*execAwareProvider{ + "python": { + fakeProvider: &fakeProvider{name: "python", displayName: "Python", shims: []string{"python"}}, + execPaths: map[string]string{"3.11.0": execPath}, + }, + } + got := runtimeExecCheckWith(filepath.Join(root, "versions"), providers).Run() + if got.OK { + t.Fatalf("expected non-OK with missing executable, got OK") + } + if got.Severity != SeverityError { + t.Errorf("severity: got %s, want error", got.Severity) + } + if got.Fixable() { + t.Errorf("runtime-executable check should be manual") + } + if !strings.Contains(got.Resolution, "dtvem install") { + t.Errorf("resolution should suggest reinstall, got %q", got.Resolution) + } +} + +func TestRuntimeExecutableCheck_OrphanedRuntimeDirIsFlagged(t *testing.T) { + root := t.TempDir() + installVersionDir(t, root, "madeup", "1.0.0") + got := runtimeExecCheckWith(filepath.Join(root, "versions"), nil).Run() + if got.OK { + t.Fatalf("expected non-OK when no provider matches install dir, got OK") + } + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, "no provider") && !strings.Contains(combined, "orphan") { + t.Errorf("expected detail to mention missing provider or orphan, got %#v", got.Details) + } +} + +func TestRuntimeExecutableCheck_DirectoryAtExecutablePathIsFlagged(t *testing.T) { + root := t.TempDir() + versionDir := installVersionDir(t, root, "python", "3.11.0") + execPath := filepath.Join(versionDir, "bin", "python") + // Plant a directory where the executable should be — corrupt + // state that's worth surfacing. + if err := os.MkdirAll(execPath, 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } + + providers := map[string]*execAwareProvider{ + "python": { + fakeProvider: &fakeProvider{name: "python", displayName: "Python", shims: []string{"python"}}, + execPaths: map[string]string{"3.11.0": execPath}, + }, + } + got := runtimeExecCheckWith(filepath.Join(root, "versions"), providers).Run() + if got.OK { + t.Fatalf("expected non-OK when a directory occupies the executable path") + } +} + +func TestRuntimeExecutableCheck_ProviderErrorIsFlagged(t *testing.T) { + root := t.TempDir() + installVersionDir(t, root, "python", "3.11.0") + + providers := map[string]*execAwareProvider{ + "python": { + fakeProvider: &fakeProvider{name: "python", displayName: "Python", shims: []string{"python"}}, + execErr: errors.New("internal provider failure"), + }, + } + got := runtimeExecCheckWith(filepath.Join(root, "versions"), providers).Run() + if got.OK { + t.Fatalf("expected non-OK when provider errors, got OK") + } + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, "internal provider failure") { + t.Errorf("expected provider error in details, got %#v", got.Details) + } +} + +func TestListInstalledVersions_MissingDirIsEmpty(t *testing.T) { + got, err := listInstalledVersions(filepath.Join(t.TempDir(), "nope")) + if err != nil { + t.Errorf("expected nil err for missing dir, got %v", err) + } + if len(got) != 0 { + t.Errorf("expected empty result, got %v", got) + } +} + +func TestListInstalledVersions_SortsByRuntimeThenVersion(t *testing.T) { + root := t.TempDir() + installVersionDir(t, root, "zruntime", "1.0.0") + installVersionDir(t, root, "aruntime", "9.9.9") + installVersionDir(t, root, "aruntime", "1.0.0") + + got, err := listInstalledVersions(filepath.Join(root, "versions")) + if err != nil { + t.Fatalf("err: %v", err) + } + want := []installedVersion{ + {runtimeName: "aruntime", version: "1.0.0"}, + {runtimeName: "aruntime", version: "9.9.9"}, + {runtimeName: "zruntime", version: "1.0.0"}, + } + if len(got) != len(want) { + t.Fatalf("len mismatch: %v vs %v", got, want) + } + for i := range got { + if got[i] != want[i] { + t.Errorf("index %d: got %+v, want %+v", i, got[i], want[i]) + } + } +} + +func TestRuntimeExecutableCheck_Registered(t *testing.T) { + found := false + for _, c := range All() { + if c.Name() == "runtime-executable-present" { + found = true + break + } + } + if !found { + t.Errorf("runtime-executable-present check is not in the default registry") + } +} diff --git a/src/internal/doctor/check_shim_binary.go b/src/internal/doctor/check_shim_binary.go new file mode 100644 index 0000000..1bf6a80 --- /dev/null +++ b/src/internal/doctor/check_shim_binary.go @@ -0,0 +1,83 @@ +package doctor + +import ( + "os" + "path/filepath" + goruntime "runtime" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" +) + +// shimBinaryCheck verifies that the dtvem-shim helper exists alongside +// the running dtvem binary. The shim binary is what gets copied into +// ~/.dtvem/shims/.exe — without it, `dtvem reshim` can't +// recreate any shims, and a fresh install can't lay down its initial +// set. +// +// The fix is to reinstall dtvem (the install scripts always ship both +// binaries together), so this check is manual: we don't try to fetch +// or rebuild the shim binary from inside dtvem itself. +type shimBinaryCheck struct { + // executable is the function used to resolve the running dtvem + // binary's path. Defaults to os.Executable; tests inject a stub so + // the check can exercise both the present and missing branches + // without having to lay down a real binary next to the test runner. + executable func() (string, error) +} + +func newShimBinaryCheck() *shimBinaryCheck { + return &shimBinaryCheck{executable: os.Executable} +} + +func (shimBinaryCheck) Name() string { return "shim-binary-present" } + +func (c shimBinaryCheck) Run() Finding { + exeFn := c.executable + if exeFn == nil { + exeFn = os.Executable + } + exe, err := exeFn() + if err != nil { + // If we can't even introspect our own location, we can't + // answer this question; report it rather than guessing. + return Finding{ + Severity: SeverityWarning, + Title: "Could not locate dtvem executable for shim-binary check", + Details: []Detail{{Key: "Error", Value: err.Error()}}, + Resolution: "This is unusual — please report it at https://github.com/CodingWithCalvin/dtvem.cli/issues", + } + } + + shimName := "dtvem-shim" + if goruntime.GOOS == constants.OSWindows { + shimName = "dtvem-shim" + constants.ExtExe + } + shimPath := filepath.Join(filepath.Dir(exe), shimName) + + if info, err := os.Stat(shimPath); err == nil && !info.IsDir() { + return Finding{OK: true, Title: "dtvem-shim binary is present"} + } + + return Finding{ + Severity: SeverityError, + Title: "dtvem-shim binary is missing", + Details: []Detail{ + {Key: "Expected at", Value: shimPath}, + }, + Resolution: shimBinaryResolution(), + } +} + +// shimBinaryResolution returns the platform-specific reinstall command. +// We point at the same installers used during initial setup so users +// don't need to discover release artifact URLs themselves. +func shimBinaryResolution() string { + if goruntime.GOOS == constants.OSWindows { + return "Reinstall dtvem to restore the shim binary:\n irm dtvem.io/install.ps1 | iex" + } + return "Reinstall dtvem to restore the shim binary:\n curl -fsSL dtvem.io/install.sh | bash" +} + +func init() { + Register(newShimBinaryCheck()) +} diff --git a/src/internal/doctor/check_shim_binary_test.go b/src/internal/doctor/check_shim_binary_test.go new file mode 100644 index 0000000..20d984e --- /dev/null +++ b/src/internal/doctor/check_shim_binary_test.go @@ -0,0 +1,139 @@ +package doctor + +import ( + "errors" + "os" + "path/filepath" + goruntime "runtime" + "strings" + "testing" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" +) + +// shimFilename is the on-disk name of the shim helper for the current +// platform, used by tests to construct realistic install layouts. +func shimFilename() string { + if goruntime.GOOS == constants.OSWindows { + return "dtvem-shim" + constants.ExtExe + } + return "dtvem-shim" +} + +func TestShimBinaryCheck_Present(t *testing.T) { + installDir := t.TempDir() + dtvemExe := filepath.Join(installDir, "dtvem") + shimExe := filepath.Join(installDir, shimFilename()) + + // The check looks for dtvem-shim next to whatever os.Executable + // returns, so we plant a real file at the expected location. + if err := os.WriteFile(shimExe, []byte("stub"), 0644); err != nil { + t.Fatalf("write shim: %v", err) + } + + c := newShimBinaryCheck() + c.executable = func() (string, error) { return dtvemExe, nil } + + got := c.Run() + if !got.OK { + t.Errorf("expected OK when shim binary exists, got %#v", got) + } +} + +func TestShimBinaryCheck_Missing(t *testing.T) { + installDir := t.TempDir() + dtvemExe := filepath.Join(installDir, "dtvem") + // Note: we deliberately do not write dtvem-shim into installDir. + + c := newShimBinaryCheck() + c.executable = func() (string, error) { return dtvemExe, nil } + + got := c.Run() + if got.OK { + t.Fatalf("expected non-OK when shim binary missing, got OK") + } + if got.Severity != SeverityError { + t.Errorf("severity: got %s, want error", got.Severity) + } + if got.Fixable() { + t.Errorf("shim-binary check should be manual, but Finding.Fix is set") + } + + // The expected location must be in details so the user knows where + // dtvem looked and didn't find it. + expectedAt := filepath.Join(installDir, shimFilename()) + foundDetail := false + for _, d := range got.Details { + if d.Value == expectedAt { + foundDetail = true + break + } + } + if !foundDetail { + t.Errorf("expected %q in details, got %#v", expectedAt, got.Details) + } +} + +func TestShimBinaryCheck_DirectoryAtExpectedPathIsTreatedAsMissing(t *testing.T) { + // Edge case: if a directory happens to share the shim's path (e.g. + // because the user manually created one), the file isn't actually + // usable, so we should report it as missing rather than present. + installDir := t.TempDir() + dtvemExe := filepath.Join(installDir, "dtvem") + shimPath := filepath.Join(installDir, shimFilename()) + if err := os.Mkdir(shimPath, 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } + + c := newShimBinaryCheck() + c.executable = func() (string, error) { return dtvemExe, nil } + + got := c.Run() + if got.OK { + t.Errorf("expected non-OK when a directory occupies the shim's path, got OK") + } +} + +func TestShimBinaryCheck_ExecutableLookupFailure(t *testing.T) { + c := newShimBinaryCheck() + c.executable = func() (string, error) { return "", errors.New("boom") } + + got := c.Run() + if got.OK { + t.Fatalf("expected non-OK finding when executable lookup fails") + } + if got.Severity != SeverityWarning { + // Warning rather than error: we couldn't run the check, but + // that doesn't necessarily mean the shim binary is missing. + t.Errorf("severity: got %s, want warning", got.Severity) + } +} + +func TestShimBinaryCheck_ResolutionPlatformSpecific(t *testing.T) { + got := shimBinaryResolution() + if !strings.Contains(got, "dtvem.io") { + t.Errorf("resolution should reference the installer URL, got %q", got) + } + if goruntime.GOOS == constants.OSWindows { + if !strings.Contains(got, "install.ps1") { + t.Errorf("Windows resolution should reference install.ps1, got %q", got) + } + } else { + if !strings.Contains(got, "install.sh") { + t.Errorf("Unix resolution should reference install.sh, got %q", got) + } + } +} + +func TestShimBinaryCheck_Registered(t *testing.T) { + found := false + for _, c := range All() { + if c.Name() == "shim-binary-present" { + found = true + break + } + } + if !found { + t.Errorf("shim-binary-present check is not in the default registry") + } +} diff --git a/src/internal/doctor/check_shim_map.go b/src/internal/doctor/check_shim_map.go new file mode 100644 index 0000000..c0cc69f --- /dev/null +++ b/src/internal/doctor/check_shim_map.go @@ -0,0 +1,242 @@ +package doctor + +import ( + "fmt" + "os" + "path/filepath" + goruntime "runtime" + "sort" + "strings" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/config" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/shim" +) + +// shimMapCheck looks for drift between the shim binaries on disk and +// the shim-map cache (~/.dtvem/cache/shim-map.json). The cache tells +// the shim executable which runtime owns each shim name; if it's +// missing entries for shim files that exist (or has entries for shims +// that have been deleted) the user sees confusing routing failures. +// +// Drift typically appears after manual file moves, a partial install +// rollback, or an older dtvem version that didn't update the cache on +// shim creation. The fix is `dtvem reshim`, which rebuilds the cache +// from disk authoritatively. +type shimMapCheck struct { + shimsDir func() string + loadShimMap func() (shim.ShimMap, error) + newManager func() (rehasher, error) +} + +func newShimMapCheck() *shimMapCheck { + return &shimMapCheck{ + shimsDir: func() string { return config.DefaultPaths().Shims }, + loadShimMap: shim.LoadShimMap, + newManager: func() (rehasher, error) { + m, err := shim.NewManager() + if err != nil { + return nil, err + } + return m, nil + }, + } +} + +func (shimMapCheck) Name() string { return "shim-map-cache" } + +func (c shimMapCheck) Run() Finding { + diskShims, err := listShimNamesOnDisk(c.shimsDir()) + if err != nil { + return Finding{ + Severity: SeverityWarning, + Title: "Could not list shim files on disk", + Details: []Detail{{Key: "Error", Value: err.Error()}}, + Resolution: "Check that " + c.shimsDir() + " is readable.", + } + } + + cache, cacheErr := c.loadShimMap() + // A missing cache file is reported by the shim package as + // os.IsNotExist; treat it as a present-but-empty cache so the + // downstream comparison reports it as drift when shim files + // exist on disk. + if cacheErr != nil && !os.IsNotExist(cacheErr) { + return Finding{ + Severity: SeverityWarning, + Title: "Could not read shim-map cache", + Details: []Detail{{Key: "Error", Value: cacheErr.Error()}}, + Resolution: "Run `dtvem reshim` to rebuild the cache from disk.", + } + } + if cache == nil { + cache = shim.ShimMap{} + } + + missingInCache, missingOnDisk := diffShimsAgainstCache(diskShims, cache) + if len(missingInCache) == 0 && len(missingOnDisk) == 0 { + // Handle the no-shims-installed case explicitly so the title + // reads correctly: "matches cache" when there's nothing on + // either side would be technically accurate but confusing. + if len(diskShims) == 0 && len(cache) == 0 { + return Finding{OK: true, Title: "No shims to check (cache and shims dir both empty)"} + } + return Finding{OK: true, Title: "Shim-map cache matches disk"} + } + + details := make([]Detail, 0, 4) + if len(missingInCache) > 0 { + details = append(details, Detail{ + Key: "On disk, missing from cache", + Value: summarizeNames(missingInCache), + }) + } + if len(missingOnDisk) > 0 { + details = append(details, Detail{ + Key: "In cache, missing on disk", + Value: summarizeNames(missingOnDisk), + }) + } + + return Finding{ + Severity: SeverityWarning, + Title: "Shim-map cache is out of sync with disk", + Details: details, + Resolution: "Run `dtvem reshim` to rebuild the cache from the installed runtimes.", + Fix: func() error { + m, err := c.newManager() + if err != nil { + return fmt.Errorf("could not create shim manager: %w", err) + } + if _, err := m.Rehash(); err != nil { + return fmt.Errorf("reshim failed: %w", err) + } + + // Rehash rebuilds the cache from the installed runtimes + // on disk, but it doesn't delete orphan shim files (shim + // binaries in shims/ whose name isn't provided by any + // installed runtime version's executable list). If we + // still see drift afterward, surface that distinctly so + // the user understands the next step is to delete the + // stray shim file by hand rather than re-running fix. + shim.ResetShimMapCache() + diskShimsAfter, listErr := listShimNamesOnDisk(c.shimsDir()) + if listErr != nil { + // We can't determine post-fix state; trust that + // reshim succeeded and let the next doctor run + // re-evaluate. Returning an error here would + // misleadingly tell the user the fix failed. + return nil //nolint:nilerr + } + cacheAfter, loadErr := c.loadShimMap() + if loadErr != nil && !os.IsNotExist(loadErr) { + return nil //nolint:nilerr + } + if cacheAfter == nil { + cacheAfter = shim.ShimMap{} + } + missingInCache, missingOnDisk := diffShimsAgainstCache(diskShimsAfter, cacheAfter) + if len(missingInCache) == 0 && len(missingOnDisk) == 0 { + return nil + } + + // Construct a focused message that names the still-broken + // shims and the manual step needed — typically `rm` on + // the orphan file, since reshim won't delete shims whose + // underlying runtime executable isn't installed anywhere. + var parts []string + if len(missingInCache) > 0 { + parts = append(parts, fmt.Sprintf("orphan shim file(s) %s — delete them from %s manually", + summarizeNames(missingInCache), c.shimsDir())) + } + if len(missingOnDisk) > 0 { + parts = append(parts, fmt.Sprintf("stale cache entrie(s) %s — re-run `dtvem reshim` after fixing the disk state", + summarizeNames(missingOnDisk))) + } + return fmt.Errorf("reshim ran but drift remains: %s", strings.Join(parts, "; ")) + }, + } +} + +// listShimNamesOnDisk returns the set of shim names present in +// shimsDir, normalized to bare names (no .exe / .cmd suffix on +// Windows) so they can be compared apples-to-apples with shim-map +// cache keys. A missing directory is not an error here: doctor calls +// this from a non-fatal check and "no shims dir" simply means an +// empty result, which the caller can interpret. +func listShimNamesOnDisk(shimsDir string) ([]string, error) { + entries, err := os.ReadDir(shimsDir) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, err + } + + seen := make(map[string]struct{}, len(entries)) + for _, e := range entries { + if e.IsDir() { + continue + } + name := e.Name() + if goruntime.GOOS == constants.OSWindows { + ext := filepath.Ext(name) + // Skip .cmd/.bat wrappers so we don't double-count a shim + // that has both a .exe and a .cmd companion. + if ext == constants.ExtCmd || ext == constants.ExtBat { + continue + } + name = strings.TrimSuffix(name, ext) + } + seen[name] = struct{}{} + } + + out := make([]string, 0, len(seen)) + for n := range seen { + out = append(out, n) + } + sort.Strings(out) + return out, nil +} + +// diffShimsAgainstCache returns the names present on disk but absent +// from the cache, and the names present in the cache but absent from +// disk. Both slices are sorted so the report output is stable across +// runs (map iteration in Go is intentionally randomized). +func diffShimsAgainstCache(diskShims []string, cache shim.ShimMap) (missingInCache, missingOnDisk []string) { + cacheSet := make(map[string]struct{}, len(cache)) + for name := range cache { + cacheSet[name] = struct{}{} + } + diskSet := make(map[string]struct{}, len(diskShims)) + for _, name := range diskShims { + diskSet[name] = struct{}{} + if _, ok := cacheSet[name]; !ok { + missingInCache = append(missingInCache, name) + } + } + for name := range cache { + if _, ok := diskSet[name]; !ok { + missingOnDisk = append(missingOnDisk, name) + } + } + sort.Strings(missingInCache) + sort.Strings(missingOnDisk) + return missingInCache, missingOnDisk +} + +// summarizeNames joins names with commas and truncates long lists so +// the rendered Detail value fits on one terminal line. We keep the +// first few items and indicate how many more were elided rather than +// trying to fit everything. +func summarizeNames(names []string) string { + const maxShown = 5 + if len(names) <= maxShown { + return strings.Join(names, ", ") + } + return fmt.Sprintf("%s, ... (+%d more)", strings.Join(names[:maxShown], ", "), len(names)-maxShown) +} + +func init() { + Register(newShimMapCheck()) +} diff --git a/src/internal/doctor/check_shim_map_test.go b/src/internal/doctor/check_shim_map_test.go new file mode 100644 index 0000000..0cc741b --- /dev/null +++ b/src/internal/doctor/check_shim_map_test.go @@ -0,0 +1,277 @@ +package doctor + +import ( + "errors" + "os" + "path/filepath" + goruntime "runtime" + "strings" + "testing" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/shim" +) + +// shimFileName produces the on-disk filename for a shim, accounting +// for Windows requiring an .exe suffix. Mirrors what shim.Manager +// writes when it creates a shim. +func shimFileName(name string) string { + if goruntime.GOOS == constants.OSWindows { + return name + constants.ExtExe + } + return name +} + +// shimMapLayout creates a temp shims directory pre-populated with the +// given shim names. Returns the directory path so a check can be +// pointed at it. +func shimMapLayout(t *testing.T, names ...string) string { + t.Helper() + dir := t.TempDir() + for _, n := range names { + f := filepath.Join(dir, shimFileName(n)) + if err := os.WriteFile(f, []byte{}, 0644); err != nil { + t.Fatalf("write shim %q: %v", n, err) + } + } + return dir +} + +// newShimMapCheckWith returns a check wired up to the provided shims +// directory and cache contents. Pass cacheErr to simulate a load +// failure that isn't a "missing file". The cache pointer can be +// reused so a test simulates rehash mutating the cache by writing +// into *cacheAfterFix from the rehasher closure. +func newShimMapCheckWith(shimsDir string, cache shim.ShimMap, cacheErr error, rh *fakeRehasher) *shimMapCheck { + c := newShimMapCheck() + c.shimsDir = func() string { return shimsDir } + c.loadShimMap = func() (shim.ShimMap, error) { return cache, cacheErr } + c.newManager = func() (rehasher, error) { + if rh == nil { + return nil, errors.New("no rehasher provided in test") + } + return rh, nil + } + return c +} + +func TestShimMapCheck_BothEmptyIsOK(t *testing.T) { + dir := shimMapLayout(t) + got := newShimMapCheckWith(dir, shim.ShimMap{}, nil, nil).Run() + if !got.OK { + t.Errorf("expected OK with empty disk and empty cache, got %#v", got) + } +} + +func TestShimMapCheck_MatchingDiskAndCacheIsOK(t *testing.T) { + dir := shimMapLayout(t, "python", "node") + cache := shim.ShimMap{ + "python": {Runtime: "python"}, + "node": {Runtime: "node"}, + } + got := newShimMapCheckWith(dir, cache, nil, nil).Run() + if !got.OK { + t.Errorf("expected OK with matching disk and cache, got %#v", got) + } +} + +func TestShimMapCheck_DriftWhenCacheMissingEntries(t *testing.T) { + dir := shimMapLayout(t, "python", "node", "ruby") + cache := shim.ShimMap{ + "python": {Runtime: "python"}, + // "node" and "ruby" are on disk but not in the cache. + } + rh := &fakeRehasher{} + got := newShimMapCheckWith(dir, cache, nil, rh).Run() + if got.OK { + t.Fatalf("expected non-OK when cache is missing entries, got OK") + } + if got.Severity != SeverityWarning { + t.Errorf("severity: got %s, want warning", got.Severity) + } + if !got.Fixable() { + t.Errorf("shim-map check should be fixable") + } + + // The missing-in-cache names must appear in the details. + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, "node") || !strings.Contains(combined, "ruby") { + t.Errorf("expected node and ruby in details, got %#v", got.Details) + } +} + +func TestShimMapCheck_DriftWhenCacheHasGhostEntries(t *testing.T) { + dir := shimMapLayout(t, "python") + cache := shim.ShimMap{ + "python": {Runtime: "python"}, + "deleted": {Runtime: "python"}, + } + rh := &fakeRehasher{} + got := newShimMapCheckWith(dir, cache, nil, rh).Run() + if got.OK { + t.Fatalf("expected non-OK when cache has entries with no disk file, got OK") + } + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, "deleted") { + t.Errorf("expected ghost cache entry 'deleted' in details, got %#v", got.Details) + } +} + +func TestShimMapCheck_FixInvokesRehash(t *testing.T) { + // Disk and cache stay in sync after the fake rehasher runs because + // we point loadShimMap at a cache that already matches the disk + // state — the rehasher itself doesn't write anything in tests. + // This isolates the "did Fix actually call Rehash?" assertion + // from the post-fix drift check. + dir := shimMapLayout(t, "python") + cacheBeforeFix := shim.ShimMap{} + cacheAfterFix := shim.ShimMap{"python": {Runtime: "python"}} + + c := newShimMapCheck() + c.shimsDir = func() string { return dir } + loadCount := 0 + c.loadShimMap = func() (shim.ShimMap, error) { + loadCount++ + if loadCount == 1 { + return cacheBeforeFix, nil + } + return cacheAfterFix, nil + } + rh := &fakeRehasher{} + c.newManager = func() (rehasher, error) { return rh, nil } + + got := c.Run() + if !got.Fixable() { + t.Fatalf("precondition: expected fixable finding") + } + if err := got.Fix(); err != nil { + t.Fatalf("Fix returned error: %v", err) + } + if !rh.called { + t.Errorf("Fix did not invoke Rehash") + } +} + +func TestShimMapCheck_FixReportsResidualDrift(t *testing.T) { + // Some drift can't be resolved by rehash alone — orphan shim + // files persist because rehash doesn't delete shims whose + // underlying runtime version doesn't actually provide that + // executable. Verify the post-fix check surfaces the residual + // drift as an error so the user doesn't see a misleading + // "Fixed" green checkmark followed by the same warning on + // the next run. + dir := shimMapLayout(t, "orphan") + emptyCache := shim.ShimMap{} + + c := newShimMapCheck() + c.shimsDir = func() string { return dir } + c.loadShimMap = func() (shim.ShimMap, error) { return emptyCache, nil } + c.newManager = func() (rehasher, error) { return &fakeRehasher{}, nil } + + got := c.Run() + if !got.Fixable() { + t.Fatalf("precondition: expected fixable finding") + } + err := got.Fix() + if err == nil { + t.Fatal("expected Fix to error when residual drift remains") + } + if !strings.Contains(err.Error(), "orphan") { + t.Errorf("expected error to mention orphan shims, got %q", err.Error()) + } +} + +func TestShimMapCheck_CacheReadErrorIsWarning(t *testing.T) { + dir := shimMapLayout(t, "python") + got := newShimMapCheckWith(dir, nil, errors.New("corrupt cache"), nil).Run() + if got.OK { + t.Fatalf("expected non-OK when cache read fails, got OK") + } + if got.Severity != SeverityWarning { + t.Errorf("severity: got %s, want warning", got.Severity) + } +} + +func TestShimMapCheck_MissingCacheFileIsTreatedAsEmpty(t *testing.T) { + // os.IsNotExist errors from LoadShimMap should be treated as a + // present-but-empty cache, which means an actual drift report + // when there are shim files on disk — not a separate warning. + dir := shimMapLayout(t, "python") + missing := &os.PathError{Op: "open", Path: "x", Err: os.ErrNotExist} + rh := &fakeRehasher{} + got := newShimMapCheckWith(dir, nil, missing, rh).Run() + if got.OK { + t.Fatalf("expected non-OK when cache is missing and shims exist, got OK") + } + if got.Severity != SeverityWarning { + t.Errorf("severity: got %s, want warning", got.Severity) + } +} + +func TestListShimNamesOnDisk_StripsExtensionsAndSkipsWrappers(t *testing.T) { + dir := t.TempDir() + // Create a .exe + .cmd pair (Windows install layout); both should + // resolve to the same logical name "python", and the function + // should not return "python" twice. + if goruntime.GOOS == constants.OSWindows { + if err := os.WriteFile(filepath.Join(dir, "python.exe"), []byte{}, 0644); err != nil { + t.Fatalf("write: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "python.cmd"), []byte{}, 0644); err != nil { + t.Fatalf("write: %v", err) + } + } else { + if err := os.WriteFile(filepath.Join(dir, "python"), []byte{}, 0755); err != nil { + t.Fatalf("write: %v", err) + } + } + + got, err := listShimNamesOnDisk(dir) + if err != nil { + t.Fatalf("listShimNamesOnDisk: %v", err) + } + if len(got) != 1 || got[0] != "python" { + t.Errorf("got %v, want [python]", got) + } +} + +func TestListShimNamesOnDisk_MissingDirIsEmpty(t *testing.T) { + got, err := listShimNamesOnDisk(filepath.Join(t.TempDir(), "nope")) + if err != nil { + t.Errorf("expected nil err for missing dir, got %v", err) + } + if len(got) != 0 { + t.Errorf("expected empty slice, got %v", got) + } +} + +func TestSummarizeNames_Truncates(t *testing.T) { + long := []string{"a", "b", "c", "d", "e", "f", "g"} + got := summarizeNames(long) + if !strings.Contains(got, "+2 more") { + t.Errorf("expected summary to elide 2 names, got %q", got) + } +} + +func TestShimMapCheck_Registered(t *testing.T) { + found := false + for _, c := range All() { + if c.Name() == "shim-map-cache" { + found = true + break + } + } + if !found { + t.Errorf("shim-map-cache check is not in the default registry") + } +} + +// detailValues extracts the string values from a slice of Details for +// easier substring assertions in the tests above. +func detailValues(ds []Detail) []string { + out := make([]string, len(ds)) + for i, d := range ds { + out[i] = d.Value + } + return out +} diff --git a/src/internal/doctor/check_shims_in_path.go b/src/internal/doctor/check_shims_in_path.go new file mode 100644 index 0000000..369ea88 --- /dev/null +++ b/src/internal/doctor/check_shims_in_path.go @@ -0,0 +1,78 @@ +package doctor + +import ( + "fmt" + "os" + "strings" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/path" +) + +// shimsInPathCheck verifies that the currently resolved shims directory +// is on $PATH. If it isn't, dtvem-managed runtimes won't be reachable +// from the shell — the shims exist but nothing routes the user's +// `python` invocation to them. +// +// The remediation is `dtvem init`, which is the same command users run +// at install time. We don't auto-invoke it here because init() writes +// to the user's shell config (Unix) or registry (Windows) and the user +// should opt in to that explicitly, separately from running `doctor`. +type shimsInPathCheck struct{} + +func newShimsInPathCheck() *shimsInPathCheck { return &shimsInPathCheck{} } + +func (shimsInPathCheck) Name() string { return "shims-in-path" } + +func (c shimsInPathCheck) Run() Finding { + shims := path.ShimsDir() + if shims == "" { + // path.ShimsDir() only returns "" when UserHomeDir fails and + // DTVEM_ROOT is unset — extremely rare, but the check would + // produce a misleading result if we treated empty as a hit, so + // we surface it as a distinct problem instead. + return Finding{ + Severity: SeverityError, + Title: "Could not resolve dtvem shims directory", + Resolution: "Set DTVEM_ROOT to your dtvem install location, or ensure your home directory is accessible.", + } + } + + if path.IsInPath(shims) { + return Finding{OK: true, Title: "dtvem shims directory is on PATH"} + } + + return Finding{ + Severity: SeverityError, + Title: "dtvem shims directory is not on PATH", + Details: []Detail{ + {Key: "Expected", Value: shims}, + {Key: "Status", Value: pathEnvSummary()}, + }, + Resolution: "Run `dtvem init` to add the shims directory to your PATH.\nIf you've already run init, restart your terminal so the PATH change takes effect.", + } +} + +// pathEnvSummary returns a short human-readable description of $PATH so +// the user can see whether PATH is genuinely missing the entry vs. just +// hasn't been reloaded in the current shell. +func pathEnvSummary() string { + pathEnv := os.Getenv("PATH") + if pathEnv == "" { + return "$PATH is empty" + } + entries := path.SplitPath(pathEnv) + // Drop empty strings produced by trailing separators so the count + // reflects real entries. + count := 0 + for _, e := range entries { + if strings.TrimSpace(e) != "" { + count++ + } + } + suffix := plural(count, "entry", "entries") + return fmt.Sprintf("%d %s in PATH, none match", count, suffix) +} + +func init() { + Register(newShimsInPathCheck()) +} diff --git a/src/internal/doctor/check_shims_in_path_test.go b/src/internal/doctor/check_shims_in_path_test.go new file mode 100644 index 0000000..f4f2641 --- /dev/null +++ b/src/internal/doctor/check_shims_in_path_test.go @@ -0,0 +1,63 @@ +package doctor + +import ( + "strings" + "testing" +) + +func TestShimsInPathCheck_Pass(t *testing.T) { + _, shims := withDtvemRoot(t) + withPathEnv(t, []string{shims, "/usr/bin"}) + + got := newShimsInPathCheck().Run() + if !got.OK { + t.Errorf("expected OK when shims dir is in PATH, got %#v", got) + } +} + +func TestShimsInPathCheck_FailWhenMissing(t *testing.T) { + _, shims := withDtvemRoot(t) + // Build a PATH that explicitly omits the shims dir. + withPathEnv(t, []string{"/usr/bin", "/usr/local/bin"}) + + got := newShimsInPathCheck().Run() + if got.OK { + t.Fatalf("expected non-OK finding when shims dir absent from PATH, got OK") + } + if got.Severity != SeverityError { + t.Errorf("severity: got %s, want error", got.Severity) + } + if got.Fixable() { + t.Errorf("shims-in-path check should be manual, but Finding.Fix is set") + } + + // The expected shims path must be in details so the user can see + // what's missing. + foundExpected := false + for _, d := range got.Details { + if d.Key == "Expected" && d.Value == shims { + foundExpected = true + break + } + } + if !foundExpected { + t.Errorf("expected shims path %q in details, got %#v", shims, got.Details) + } + + if !strings.Contains(got.Resolution, "dtvem init") { + t.Errorf("resolution should reference `dtvem init`, got %q", got.Resolution) + } +} + +func TestShimsInPathCheck_Registered(t *testing.T) { + found := false + for _, c := range All() { + if c.Name() == "shims-in-path" { + found = true + break + } + } + if !found { + t.Errorf("shims-in-path check is not in the default registry") + } +} diff --git a/src/internal/doctor/check_stale_shims.go b/src/internal/doctor/check_stale_shims.go new file mode 100644 index 0000000..1374399 --- /dev/null +++ b/src/internal/doctor/check_stale_shims.go @@ -0,0 +1,162 @@ +package doctor + +import ( + "fmt" + "os" + goruntime "runtime" + "strings" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/path" +) + +// staleShimsCheck looks for entries in $PATH that match the dtvem +// shims-directory pattern but don't equal the current resolved shims +// directory. These are usually left over from a pre-XDG install or +// from switching install types (system vs. user PATH on Windows). +// +// The check is fixable: on Windows we surgically remove stale entries +// from the User PATH registry value (no admin needed); on Unix we +// rewrite the user's shell config to drop "# Added by dtvem"-marked +// blocks that point at stale paths. System PATH stale entries on +// Windows still require admin and remain a manual step; we surface +// that explicitly in the report when applicable. +type staleShimsCheck struct { + // Injected for testability. Each closure defaults to a real + // implementation; tests override individually so the check can be + // exercised across success, no-op, and platform-specific paths + // without touching the actual registry or shell config. + pathEnv func() string + currentShimsDir func() string + userHomeDir func() (string, error) + removeFromPath func(currentShimsDir string) ([]string, error) + removeFromFile func(configFile, currentShimsDir string) ([]string, error) + detectShell func() string + shellConfigFile func(shell string) string +} + +func newStaleShimsCheck() *staleShimsCheck { + return &staleShimsCheck{ + pathEnv: func() string { return os.Getenv("PATH") }, + currentShimsDir: path.ShimsDir, + userHomeDir: os.UserHomeDir, + removeFromPath: removeStaleShimsFromUserPath, + removeFromFile: path.RemoveStaleShimsFromShellConfig, + detectShell: path.DetectShell, + shellConfigFile: path.GetShellConfigFile, + } +} + +func (staleShimsCheck) Name() string { return "stale-shims-path" } + +func (c *staleShimsCheck) Run() Finding { + currentShims := c.currentShimsDir() + pathEntries := path.SplitPath(c.pathEnv()) + stale := path.FindStaleShimsEntries(pathEntries, currentShims) + + if len(stale) == 0 { + return Finding{OK: true, Title: "No stale dtvem shims entries in PATH"} + } + + details := []Detail{{Key: "Expected", Value: currentShims}} + for _, s := range stale { + details = append(details, Detail{Key: "Found", Value: s}) + } + + resolution, fix := c.fixOrInstructions(stale, currentShims) + return Finding{ + Severity: SeverityError, + Title: fmt.Sprintf("Stale dtvem shims entr%s in PATH", plural(len(stale), "y", "ies")), + Details: details, + Resolution: resolution, + Fix: fix, + } +} + +// fixOrInstructions returns the resolution text and (optionally) a Fix +// closure for the current platform. On both platforms we always return +// instructions, but only return a Fix when we have a safe in-place +// remediation: User PATH registry rewrite on Windows, shell-config +// marker-block removal on Unix. +func (c *staleShimsCheck) fixOrInstructions(stale []string, currentShims string) (string, func() error) { + if goruntime.GOOS == constants.OSWindows { + return c.windowsFix(stale, currentShims) + } + return c.unixFix(currentShims) +} + +func (c *staleShimsCheck) windowsFix(stale []string, currentShims string) (string, func() error) { + resolution := strings.Join([]string{ + "Remove the stale entries from your User PATH (no admin required).", + "If the entries are in System PATH, you'll need to run this from an", + "elevated terminal — doctor will only touch User PATH automatically.", + "Restart your terminal after the fix for the change to take effect.", + }, "\n") + + fix := func() error { + removed, err := c.removeFromPath(currentShims) + if err != nil { + return err + } + if len(removed) == 0 { + // User PATH didn't actually contain the stale entries, so + // they must be in System PATH (which we can't write to + // without admin). Surface that distinctly rather than + // silently succeeding — the user would otherwise expect + // the warning to disappear and be confused when it doesn't. + return fmt.Errorf("no stale entries found in User PATH; %d entr%s likely in System PATH and require an elevated terminal", + len(stale), plural(len(stale), "y is", "ies are")) + } + return nil + } + return resolution, fix +} + +func (c *staleShimsCheck) unixFix(currentShims string) (string, func() error) { + shell := c.detectShell() + configFile := c.shellConfigFile(shell) + if configFile == "" { + return strings.Join([]string{ + "Could not auto-detect your shell config file; edit your shell rc", + "manually and remove the export line(s) that reference the stale", + "'Found' paths above. Restart your terminal afterward.", + }, "\n"), nil + } + + resolution := strings.Join([]string{ + "Remove the '# Added by dtvem' marker block(s) referencing stale paths from", + " " + configFile, + "After the fix, restart your terminal or 'source' the file.", + }, "\n") + + fix := func() error { + removed, err := c.removeFromFile(configFile, currentShims) + if err != nil { + return err + } + if len(removed) == 0 { + // The export must live somewhere we don't auto-detect (a + // custom config file, /etc/profile, etc.). Tell the user + // what we looked at so they know where to keep searching. + return fmt.Errorf("no '# Added by dtvem' marker blocks found in %s; the stale export(s) may live in another config file", configFile) + } + return nil + } + return resolution, fix +} + +// removeStaleShimsFromUserPath is the default Windows-side fix used by +// the staleShimsCheck. The actual registry write lives in +// internal/path/path_windows.go; on Unix builds the impl returns a +// sentinel error since the cross-platform code path in staleShimsCheck +// doesn't reach this branch. +// +// This indirection lets the check stay compile-time portable while +// keeping the real implementation behind build tags where it belongs. +func removeStaleShimsFromUserPath(currentShimsDir string) ([]string, error) { + return removeStaleShimsFromUserPathImpl(currentShimsDir) +} + +func init() { + Register(newStaleShimsCheck()) +} diff --git a/src/internal/doctor/check_stale_shims_other.go b/src/internal/doctor/check_stale_shims_other.go new file mode 100644 index 0000000..b773c96 --- /dev/null +++ b/src/internal/doctor/check_stale_shims_other.go @@ -0,0 +1,21 @@ +//go:build !windows + +package doctor + +import "errors" + +// errNotWindows is returned by the Unix stub of +// removeStaleShimsFromUserPathImpl. It exists as a named sentinel so +// callers can identify the "called on the wrong platform" branch +// without asserting on error text. +var errNotWindows = errors.New("registry-based PATH fix is Windows-only") + +// removeStaleShimsFromUserPathImpl is unreachable from the stale-shims +// check's Unix branch — the check only routes here on Windows — but +// Go still needs a definition for the symbol to compile. We return a +// distinguishable sentinel error so any future code path that +// accidentally calls it on Unix surfaces the mistake instead of +// silently returning success. +func removeStaleShimsFromUserPathImpl(string) ([]string, error) { + return nil, errNotWindows +} diff --git a/src/internal/doctor/check_stale_shims_test.go b/src/internal/doctor/check_stale_shims_test.go new file mode 100644 index 0000000..9b1426d --- /dev/null +++ b/src/internal/doctor/check_stale_shims_test.go @@ -0,0 +1,244 @@ +package doctor + +import ( + "errors" + "os" + "path/filepath" + goruntime "runtime" + "strings" + "testing" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" +) + +// withPathEnv sets $PATH for the duration of t and restores it after. +// Tests rely on this to construct a known PATH list without depending +// on whatever the developer's shell happens to be exporting. +func withPathEnv(t *testing.T, entries []string) { + t.Helper() + sep := ":" + if goruntime.GOOS == constants.OSWindows { + sep = ";" + } + original := os.Getenv("PATH") + t.Cleanup(func() { _ = os.Setenv("PATH", original) }) + _ = os.Setenv("PATH", strings.Join(entries, sep)) +} + +// withDtvemRoot points DTVEM_ROOT at a temp dir so path.ShimsDir() +// resolves to a known, test-controlled location. Returns the resolved +// shims directory the test can compare against. +func withDtvemRoot(t *testing.T) (root, shims string) { + t.Helper() + root = t.TempDir() + original := os.Getenv("DTVEM_ROOT") + t.Cleanup(func() { _ = os.Setenv("DTVEM_ROOT", original) }) + _ = os.Setenv("DTVEM_ROOT", root) + return root, filepath.Join(root, "shims") +} + +func TestStaleShimsCheck_NoStaleEntries(t *testing.T) { + _, shims := withDtvemRoot(t) + withPathEnv(t, []string{shims, "/usr/local/bin"}) + + got := newStaleShimsCheck().Run() + if !got.OK { + t.Errorf("expected OK finding with only current shims in PATH, got %#v", got) + } +} + +func TestStaleShimsCheck_DetectsStaleEntry(t *testing.T) { + _, shims := withDtvemRoot(t) + + // Construct a path that matches the dtvem shims pattern but is + // different from the current resolved shims dir. + stale := filepath.Join(t.TempDir(), ".dtvem", "shims") + withPathEnv(t, []string{stale, shims, "/usr/local/bin"}) + + got := newStaleShimsCheck().Run() + if got.OK { + t.Fatalf("expected non-OK finding when PATH contains stale shims entry, got OK") + } + if got.Severity != SeverityError { + t.Errorf("severity: got %s, want error", got.Severity) + } + if !got.Fixable() { + t.Errorf("stale-shims check should be fixable (Finding.Fix should be set)") + } + + // The stale path must appear in the details so the user can find it. + foundStale := false + for _, d := range got.Details { + if d.Key == "Found" && d.Value == stale { + foundStale = true + break + } + } + if !foundStale { + t.Errorf("expected stale path %q in Details, got %#v", stale, got.Details) + } +} + +func TestStaleShimsCheck_MultipleStaleEntriesAreAllReported(t *testing.T) { + _, shims := withDtvemRoot(t) + stale1 := filepath.Join(t.TempDir(), ".dtvem", "shims") + stale2 := filepath.Join(t.TempDir(), "dtvem", "shims") + withPathEnv(t, []string{stale1, stale2, shims}) + + got := newStaleShimsCheck().Run() + if got.OK { + t.Fatalf("expected non-OK finding, got OK") + } + + // Both stale entries should be present in Details. + values := make(map[string]bool) + for _, d := range got.Details { + if d.Key == "Found" { + values[d.Value] = true + } + } + if !values[stale1] || !values[stale2] { + t.Errorf("expected both stale entries in details, got values=%v", values) + } + + // Title should pluralize for multiple entries. + if !strings.Contains(got.Title, "entries") { + t.Errorf("title should pluralize for >1 stale entries, got %q", got.Title) + } +} + +func TestStaleShimsCheck_Registered(t *testing.T) { + // The init() function should add the stale-shims check to the + // package-level registry. We confirm by name rather than instance + // so the check can be re-implemented without breaking this test. + found := false + for _, c := range All() { + if c.Name() == "stale-shims-path" { + found = true + break + } + } + if !found { + t.Errorf("stale-shims-path check is not in the default registry") + } +} + +func TestPlural(t *testing.T) { + if got := plural(1, "y", "ies"); got != "y" { + t.Errorf("plural(1) = %q, want %q", got, "y") + } + if got := plural(2, "y", "ies"); got != "ies" { + t.Errorf("plural(2) = %q, want %q", got, "ies") + } + if got := plural(0, "y", "ies"); got != "ies" { + t.Errorf("plural(0) = %q, want %q", got, "ies") + } +} + +// staleShimsTestCheck returns a staleShimsCheck wired with all fields +// injected so each test can drive both platform branches deterministically +// without depending on host env vars or the real registry/filesystem. +func staleShimsTestCheck(stalePath, current string, removeFromPath, removeFromFile func(string, string) ([]string, error)) *staleShimsCheck { + c := newStaleShimsCheck() + c.pathEnv = func() string { + // Use an OS-specific separator so SplitPath round-trips. + sep := ":" + if goruntimeIsWindows() { + sep = ";" + } + return stalePath + sep + current + } + c.currentShimsDir = func() string { return current } + if removeFromPath != nil { + c.removeFromPath = func(s string) ([]string, error) { return removeFromPath(s, current) } + } + if removeFromFile != nil { + c.removeFromFile = removeFromFile + } + c.detectShell = func() string { return "bash" } + c.shellConfigFile = func(_ string) string { return "/tmp/test-config" } + return c +} + +// goruntimeIsWindows mirrors runtime.GOOS == "windows" without +// importing the alias in this test file. +func goruntimeIsWindows() bool { + return os.PathSeparator == '\\' +} + +func TestStaleShimsCheck_FixCallsPlatformImpl(t *testing.T) { + // Build a layout with one stale path and one current entry. + root := t.TempDir() + current := filepath.Join(root, "shims") + stale := filepath.Join(t.TempDir(), ".dtvem", "shims") + + var pathCalled, fileCalled bool + pathFix := func(_, _ string) ([]string, error) { + pathCalled = true + return []string{stale}, nil + } + fileFix := func(_, _ string) ([]string, error) { + fileCalled = true + return []string{stale}, nil + } + + c := staleShimsTestCheck(stale, current, pathFix, fileFix) + got := c.Run() + if !got.Fixable() { + t.Fatalf("precondition: expected fixable finding") + } + if err := got.Fix(); err != nil { + t.Fatalf("Fix returned error: %v", err) + } + + if goruntimeIsWindows() { + if !pathCalled { + t.Errorf("Windows fix should call removeFromPath, but didn't") + } + if fileCalled { + t.Errorf("Windows fix unexpectedly called removeFromFile") + } + } else { + if !fileCalled { + t.Errorf("Unix fix should call removeFromFile, but didn't") + } + if pathCalled { + t.Errorf("Unix fix unexpectedly called removeFromPath") + } + } +} + +func TestStaleShimsCheck_FixReportsWhenNothingRemoved(t *testing.T) { + // If the impl reports zero removals, the Fix should surface that + // as an error so the user understands the cleanup didn't happen — + // rather than silently succeeding and leaving them wondering why + // the warning persists. + root := t.TempDir() + current := filepath.Join(root, "shims") + stale := filepath.Join(t.TempDir(), ".dtvem", "shims") + + zero := func(_, _ string) ([]string, error) { return nil, nil } + c := staleShimsTestCheck(stale, current, zero, zero) + got := c.Run() + if !got.Fixable() { + t.Fatalf("precondition: expected fixable finding") + } + err := got.Fix() + if err == nil { + t.Fatal("expected error when impl removed nothing") + } +} + +func TestStaleShimsCheck_FixPropagatesImplError(t *testing.T) { + root := t.TempDir() + current := filepath.Join(root, "shims") + stale := filepath.Join(t.TempDir(), ".dtvem", "shims") + + wantErr := errors.New("simulated registry failure") + bad := func(_, _ string) ([]string, error) { return nil, wantErr } + c := staleShimsTestCheck(stale, current, bad, bad) + got := c.Run() + if err := got.Fix(); !errors.Is(err, wantErr) { + t.Errorf("Fix err = %v, want chain containing %v", err, wantErr) + } +} diff --git a/src/internal/doctor/check_stale_shims_windows.go b/src/internal/doctor/check_stale_shims_windows.go new file mode 100644 index 0000000..a016f1d --- /dev/null +++ b/src/internal/doctor/check_stale_shims_windows.go @@ -0,0 +1,13 @@ +//go:build windows + +package doctor + +import "github.com/CodingWithCalvin/dtvem.cli/src/internal/path" + +// removeStaleShimsFromUserPathImpl forwards to the Windows-only +// registry helper. Wrapping it through this file lets the check's +// default-wiring constructor stay platform-agnostic — the staleShims +// check imports nothing from the registry directly. +func removeStaleShimsFromUserPathImpl(currentShimsDir string) ([]string, error) { + return path.RemoveStaleShimsFromUserPath(currentShimsDir) +} diff --git a/src/internal/doctor/check_system_runtime.go b/src/internal/doctor/check_system_runtime.go new file mode 100644 index 0000000..65bff49 --- /dev/null +++ b/src/internal/doctor/check_system_runtime.go @@ -0,0 +1,217 @@ +package doctor + +import ( + "fmt" + "os" + "path/filepath" + goruntime "runtime" + "sort" + "strings" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/path" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/runtime" +) + +// systemRuntimeCheck looks for system-installed runtimes that win +// shell resolution against dtvem's shims because their containing +// directory appears in $PATH ahead of the shims directory. When that +// happens the user's `python` (or `node`, etc.) invocation routes to +// the system binary rather than the dtvem-managed version, often +// silently — the bug isn't that dtvem failed, it's that the shell +// never asked dtvem. +// +// This is manual: fixing it means either uninstalling the system +// runtime or reordering PATH, both of which are intent decisions the +// user needs to make rather than something doctor should automate. +type systemRuntimeCheck struct { + // Injected for testability. listProviders defaults to the global + // runtime registry; pathEnv and shimsDir default to the live + // environment. Each gets a stub in tests so the check can run + // against synthetic PATH layouts without depending on whatever + // runtimes happen to be installed on the test host. + listProviders func() []runtime.ShimProvider + pathEnv func() string + shimsDir func() string +} + +func newSystemRuntimeCheck() *systemRuntimeCheck { + return &systemRuntimeCheck{ + listProviders: runtime.GetAllShimProviders, + pathEnv: func() string { return os.Getenv("PATH") }, + shimsDir: path.ShimsDir, + } +} + +func (systemRuntimeCheck) Name() string { return "system-runtime-precedence" } + +func (c systemRuntimeCheck) Run() Finding { + providers := c.listProviders() + if len(providers) == 0 { + // No registered runtime providers means we can't form a list + // of shim names to check; treat as a passing finding rather + // than misleadingly reporting "no conflicts found". + return Finding{OK: true, Title: "No runtime providers registered (nothing to check)"} + } + + shimsDir := c.shimsDir() + pathEntries := path.SplitPath(c.pathEnv()) + + // Find where the shims dir sits in PATH so we can compare other + // directories' positions against it. A shims dir absent from PATH + // is already surfaced by shimsInPathCheck, so we skip this whole + // check rather than double-reporting. + shimsPos := indexOfPathEntry(pathEntries, shimsDir) + if shimsPos < 0 { + return Finding{OK: true, Title: "Shims dir not on PATH (covered by another check)"} + } + + conflicts := findRuntimeConflicts(providers, pathEntries, shimsPos) + if len(conflicts) == 0 { + return Finding{OK: true, Title: "No system runtimes ahead of dtvem on PATH"} + } + + // Sort by runtime name so the report is stable across runs even + // when the conflicts map is iterated in random order internally. + sort.Slice(conflicts, func(i, j int) bool { + return conflicts[i].runtimeName < conflicts[j].runtimeName + }) + + details := make([]Detail, 0, len(conflicts)) + for _, c := range conflicts { + details = append(details, Detail{ + Key: c.displayName, + Value: fmt.Sprintf("%s (matches: %s)", c.dir, strings.Join(c.matches, ", ")), + }) + } + + return Finding{ + Severity: SeverityWarning, + Title: fmt.Sprintf("System runtime%s on PATH before dtvem shims", plural(len(conflicts), "", "s")), + Details: details, + Resolution: systemRuntimeResolution(), + } +} + +// runtimeConflict pairs a runtime's display name with the offending +// PATH directory and the specific shim names that resolved to it. +// "matches" is plural because most runtimes ship more than one +// executable (python + python3 + pip + pip3) and a single offending +// directory typically holds several of them at once. +type runtimeConflict struct { + runtimeName string + displayName string + dir string + matches []string +} + +// findRuntimeConflicts returns one runtimeConflict per registered +// runtime that has at least one shim name appearing in a PATH +// directory positioned before shimsPos. The matched shim names are +// deduplicated and order-preserved. +func findRuntimeConflicts(providers []runtime.ShimProvider, pathEntries []string, shimsPos int) []runtimeConflict { + var conflicts []runtimeConflict + for _, p := range providers { + // For each runtime, scan PATH entries that come BEFORE the + // shims dir. The first non-shims dir containing any of the + // runtime's executables is the offender. Stopping at the + // first match avoids reporting every subsequent system dir + // for the same runtime, which would just be noise. + var firstDir string + var matches []string + for i, dir := range pathEntries[:shimsPos] { + _ = i + trimmed := strings.TrimSpace(dir) + if trimmed == "" || isSameDir(trimmed, pathEntries[shimsPos]) { + continue + } + localMatches := matchingExecutables(trimmed, p.Shims()) + if len(localMatches) == 0 { + continue + } + firstDir = trimmed + matches = localMatches + break + } + if firstDir != "" { + conflicts = append(conflicts, runtimeConflict{ + runtimeName: p.Name(), + displayName: p.DisplayName(), + dir: firstDir, + matches: matches, + }) + } + } + return conflicts +} + +// matchingExecutables returns the subset of shimNames that have a +// matching executable file in dir. Platform-specific extension rules +// mirror what the shim manager uses when populating its cache. +func matchingExecutables(dir string, shimNames []string) []string { + var matches []string + for _, name := range shimNames { + if executableExistsInDir(dir, name) { + matches = append(matches, name) + } + } + return matches +} + +// executableExistsInDir reports whether an executable named name is +// present in dir, using the same .exe/.cmd/.bat probing the rest of +// the codebase does so we don't disagree about what "executable +// exists" means across packages. +func executableExistsInDir(dir, name string) bool { + if goruntime.GOOS == constants.OSWindows { + for _, ext := range []string{constants.ExtExe, constants.ExtCmd, constants.ExtBat} { + if info, err := os.Stat(filepath.Join(dir, name+ext)); err == nil && !info.IsDir() { + return true + } + } + return false + } + info, err := os.Stat(filepath.Join(dir, name)) + if err != nil || info.IsDir() { + return false + } + return info.Mode()&0111 != 0 +} + +// indexOfPathEntry returns the index of needle in haystack, comparing +// case-insensitively on Windows. Returns -1 when not found. +func indexOfPathEntry(haystack []string, needle string) int { + needleClean := filepath.Clean(needle) + for i, h := range haystack { + if strings.TrimSpace(h) == "" { + continue + } + if isSameDir(filepath.Clean(h), needleClean) { + return i + } + } + return -1 +} + +// isSameDir reports whether two cleaned directory paths refer to the +// same location, applying case-insensitive comparison on Windows +// since the registry can hand us mixed casing for the same path. +func isSameDir(a, b string) bool { + if goruntime.GOOS == constants.OSWindows { + return strings.EqualFold(a, b) + } + return a == b +} + +func systemRuntimeResolution() string { + return strings.Join([]string{ + "Options:", + " 1. Uninstall the system runtime so dtvem's shim takes over.", + " 2. Move the offending directory below the dtvem shims directory in PATH.", + " 3. Accept the system runtime as your default and remove dtvem's shim for that runtime.", + }, "\n") +} + +func init() { + Register(newSystemRuntimeCheck()) +} diff --git a/src/internal/doctor/check_system_runtime_test.go b/src/internal/doctor/check_system_runtime_test.go new file mode 100644 index 0000000..7d73771 --- /dev/null +++ b/src/internal/doctor/check_system_runtime_test.go @@ -0,0 +1,196 @@ +package doctor + +import ( + "os" + "path/filepath" + goruntime "runtime" + "strings" + "testing" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/runtime" +) + +// fakeProvider is a runtime.ShimProvider stand-in used by these tests +// to avoid pulling in the real node/python/ruby providers (which would +// transitively import HTTP fetchers, manifests, etc.). +type fakeProvider struct { + name string + displayName string + shims []string +} + +func (f *fakeProvider) Name() string { return f.name } +func (f *fakeProvider) DisplayName() string { return f.displayName } +func (f *fakeProvider) Shims() []string { return f.shims } +func (f *fakeProvider) ExecutablePath(string) (string, error) { return "", nil } +func (f *fakeProvider) IsInstalled(string) (bool, error) { return false, nil } +func (f *fakeProvider) ShouldReshimAfter(string, []string) bool { return false } +func (f *fakeProvider) GetEnvironment(string) (map[string]string, error) { + return map[string]string{}, nil +} + +// writeExecutable creates a file at dir/name with platform-appropriate +// suffix and execute permission. Returns the full path. +func writeExecutable(t *testing.T, dir, name string) string { + t.Helper() + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } + full := filepath.Join(dir, name) + if goruntime.GOOS == constants.OSWindows { + full = filepath.Join(dir, name+constants.ExtExe) + } + if err := os.WriteFile(full, []byte("stub"), 0755); err != nil { + t.Fatalf("write %s: %v", full, err) + } + return full +} + +// systemRuntimeTestCheck returns a check wired up to deterministic +// PATH entries and provider list. +func systemRuntimeTestCheck(pathEntries []string, shimsDir string, providers []runtime.ShimProvider) *systemRuntimeCheck { + c := newSystemRuntimeCheck() + sep := ":" + if goruntime.GOOS == constants.OSWindows { + sep = ";" + } + c.pathEnv = func() string { return strings.Join(pathEntries, sep) } + c.shimsDir = func() string { return shimsDir } + c.listProviders = func() []runtime.ShimProvider { return providers } + return c +} + +func TestSystemRuntimeCheck_NoProvidersIsOK(t *testing.T) { + got := systemRuntimeTestCheck(nil, "/shims", nil).Run() + if !got.OK { + t.Errorf("expected OK with no providers, got %#v", got) + } +} + +func TestSystemRuntimeCheck_ShimsDirNotInPathIsOK(t *testing.T) { + // When the shims dir isn't on PATH at all, this check defers to + // shimsInPathCheck and reports OK rather than double-flagging. + providers := []runtime.ShimProvider{ + &fakeProvider{name: "python", displayName: "Python", shims: []string{"python"}}, + } + got := systemRuntimeTestCheck([]string{"/usr/bin"}, "/shims", providers).Run() + if !got.OK { + t.Errorf("expected OK when shims dir absent, got %#v", got) + } +} + +func TestSystemRuntimeCheck_DetectsSystemPythonAhead(t *testing.T) { + tmp := t.TempDir() + systemDir := filepath.Join(tmp, "system") + shimsDir := filepath.Join(tmp, "shims") + if err := os.MkdirAll(shimsDir, 0755); err != nil { + t.Fatalf("mkdir shims: %v", err) + } + writeExecutable(t, systemDir, "python") + + providers := []runtime.ShimProvider{ + &fakeProvider{name: "python", displayName: "Python", shims: []string{"python", "pip"}}, + } + got := systemRuntimeTestCheck([]string{systemDir, shimsDir}, shimsDir, providers).Run() + if got.OK { + t.Fatalf("expected non-OK when system python is ahead, got OK") + } + if got.Severity != SeverityWarning { + t.Errorf("severity: got %s, want warning", got.Severity) + } + if got.Fixable() { + t.Errorf("system-runtime check should be manual, but Finding.Fix is set") + } + + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, systemDir) { + t.Errorf("expected system dir %q in details, got %#v", systemDir, got.Details) + } + if !strings.Contains(combined, "python") { + t.Errorf("expected matched shim 'python' in details, got %#v", got.Details) + } +} + +func TestSystemRuntimeCheck_NoConflictWhenSystemDirIsAfterShims(t *testing.T) { + tmp := t.TempDir() + systemDir := filepath.Join(tmp, "system") + shimsDir := filepath.Join(tmp, "shims") + if err := os.MkdirAll(shimsDir, 0755); err != nil { + t.Fatalf("mkdir shims: %v", err) + } + writeExecutable(t, systemDir, "python") + + providers := []runtime.ShimProvider{ + &fakeProvider{name: "python", displayName: "Python", shims: []string{"python"}}, + } + // shimsDir comes first → dtvem wins → no conflict. + got := systemRuntimeTestCheck([]string{shimsDir, systemDir}, shimsDir, providers).Run() + if !got.OK { + t.Errorf("expected OK when shims dir precedes system dir, got %#v", got) + } +} + +func TestSystemRuntimeCheck_StopsAtFirstMatchingDirPerRuntime(t *testing.T) { + tmp := t.TempDir() + firstDir := filepath.Join(tmp, "first") + secondDir := filepath.Join(tmp, "second") + shimsDir := filepath.Join(tmp, "shims") + if err := os.MkdirAll(shimsDir, 0755); err != nil { + t.Fatalf("mkdir shims: %v", err) + } + writeExecutable(t, firstDir, "python") + writeExecutable(t, secondDir, "python") + + providers := []runtime.ShimProvider{ + &fakeProvider{name: "python", displayName: "Python", shims: []string{"python"}}, + } + got := systemRuntimeTestCheck([]string{firstDir, secondDir, shimsDir}, shimsDir, providers).Run() + if got.OK { + t.Fatal("expected non-OK") + } + // Only the first dir should appear in details, not the second. + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, firstDir) { + t.Errorf("first dir should be in details: %#v", got.Details) + } + if strings.Contains(combined, secondDir) { + t.Errorf("second dir should NOT be in details (only first match per runtime): %#v", got.Details) + } +} + +func TestSystemRuntimeCheck_MultipleRuntimes(t *testing.T) { + tmp := t.TempDir() + systemDir := filepath.Join(tmp, "system") + shimsDir := filepath.Join(tmp, "shims") + if err := os.MkdirAll(shimsDir, 0755); err != nil { + t.Fatalf("mkdir shims: %v", err) + } + writeExecutable(t, systemDir, "python") + writeExecutable(t, systemDir, "node") + + providers := []runtime.ShimProvider{ + &fakeProvider{name: "python", displayName: "Python", shims: []string{"python"}}, + &fakeProvider{name: "node", displayName: "Node.js", shims: []string{"node"}}, + } + got := systemRuntimeTestCheck([]string{systemDir, shimsDir}, shimsDir, providers).Run() + if got.OK { + t.Fatalf("expected non-OK with two conflicting runtimes") + } + if len(got.Details) != 2 { + t.Errorf("expected 2 details (one per runtime), got %d: %#v", len(got.Details), got.Details) + } +} + +func TestSystemRuntimeCheck_Registered(t *testing.T) { + found := false + for _, c := range All() { + if c.Name() == "system-runtime-precedence" { + found = true + break + } + } + if !found { + t.Errorf("system-runtime-precedence check is not in the default registry") + } +} diff --git a/src/internal/doctor/check_xdg_split.go b/src/internal/doctor/check_xdg_split.go new file mode 100644 index 0000000..b1f6086 --- /dev/null +++ b/src/internal/doctor/check_xdg_split.go @@ -0,0 +1,179 @@ +package doctor + +import ( + "os" + "path/filepath" + goruntime "runtime" + "strings" + + "github.com/CodingWithCalvin/dtvem.cli/src/internal/config" + "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" +) + +// xdgSplitCheck detects the case where dtvem has data installed at +// more than one of its candidate root locations. The classic split +// is: a user originally installed dtvem when XDG_DATA_HOME was unset +// (so versions landed under ~/.dtvem), then later set XDG_DATA_HOME +// and reinstalled — the active install now lives elsewhere, but +// shims/configs/versions in ~/.dtvem still exist and clutter the +// system or, worse, get used by an outdated dtvem binary. +// +// We don't attempt to auto-migrate: that's an intent decision (which +// data to keep, whether to consolidate to XDG or back to ~/.dtvem) +// and it's destructive enough to deserve an explicit user choice. +type xdgSplitCheck struct { + // Injected so tests can drive XDG/home/DTVEM_ROOT permutations + // deterministically without mutating real env vars and + // directories. + activeRoot func() string + candidateRoots func() []string + hasData func(root string) bool +} + +func newXDGSplitCheck() *xdgSplitCheck { + return &xdgSplitCheck{ + activeRoot: func() string { return config.DefaultPaths().Root }, + candidateRoots: candidateDtvemRoots, + hasData: rootHasData, + } +} + +func (xdgSplitCheck) Name() string { return "xdg-split-state" } + +func (c xdgSplitCheck) Run() Finding { + active := c.activeRoot() + cleanActive := filepath.Clean(active) + + var orphans []string + for _, candidate := range c.candidateRoots() { + if candidate == "" { + continue + } + cleanCandidate := filepath.Clean(candidate) + if pathsEqual(cleanCandidate, cleanActive) { + continue + } + if c.hasData(candidate) { + orphans = append(orphans, candidate) + } + } + + if len(orphans) == 0 { + return Finding{OK: true, Title: "Only one dtvem install location holds data"} + } + + details := []Detail{{Key: "Active install", Value: active}} + for _, o := range orphans { + details = append(details, Detail{Key: "Stale install", Value: o}) + } + + return Finding{ + Severity: SeverityWarning, + Title: "dtvem data exists at more than one install location", + Details: details, + Resolution: xdgSplitResolution(), + } +} + +// candidateDtvemRoots returns every directory where a dtvem install +// could plausibly have data: the DTVEM_ROOT override, the XDG-derived +// path, and the home-relative ~/.dtvem path. Order is intentional — +// active root first — so the caller can dedupe trivially against +// activeRoot. +func candidateDtvemRoots() []string { + var out []string + + if v := os.Getenv("DTVEM_ROOT"); v != "" { + out = append(out, v) + } + + if xdg := os.Getenv("XDG_DATA_HOME"); xdg != "" { + out = append(out, filepath.Join(xdg, "dtvem")) + } + + if home, err := os.UserHomeDir(); err == nil && home != "" { + // ~/.dtvem (macOS/Windows default, also a common pre-XDG + // Linux layout). + out = append(out, filepath.Join(home, ".dtvem")) + // ~/.local/share/dtvem (Linux XDG default with XDG_DATA_HOME + // unset, and a common alternate spot if the user switches + // XDG_DATA_HOME between sessions). + if goruntime.GOOS == constants.OSLinux { + out = append(out, filepath.Join(home, ".local", "share", "dtvem")) + } + } + + return dedupePaths(out) +} + +// rootHasData reports whether root looks like a populated dtvem +// install. We require at least one runtime version directory to +// avoid flagging brand-new installs that have created the directory +// tree but not yet downloaded anything — those aren't "data" the +// user could reasonably reuse or care about migrating. +func rootHasData(root string) bool { + versionsDir := filepath.Join(root, "versions") + entries, err := os.ReadDir(versionsDir) + if err != nil { + return false + } + for _, runtime := range entries { + if !runtime.IsDir() { + continue + } + versions, err := os.ReadDir(filepath.Join(versionsDir, runtime.Name())) + if err != nil { + continue + } + for _, v := range versions { + if v.IsDir() { + return true + } + } + } + return false +} + +// dedupePaths returns paths with duplicates removed, comparing +// case-insensitively on Windows. Order of first occurrence is +// preserved. +func dedupePaths(paths []string) []string { + seen := make(map[string]struct{}, len(paths)) + out := make([]string, 0, len(paths)) + for _, p := range paths { + key := filepath.Clean(p) + if goruntime.GOOS == constants.OSWindows { + key = strings.ToLower(key) + } + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + out = append(out, p) + } + return out +} + +// pathsEqual reports whether two cleaned paths refer to the same +// directory, with case-insensitive comparison on Windows. +func pathsEqual(a, b string) bool { + if goruntime.GOOS == constants.OSWindows { + return strings.EqualFold(a, b) + } + return a == b +} + +func xdgSplitResolution() string { + return strings.Join([]string{ + "Pick one install location and consolidate:", + " - If the 'Active install' is the one you want to keep, delete the 'Stale install' tree(s)", + " after moving any versions you still need across with `dtvem install`.", + " - If a 'Stale install' is actually the one you want to keep, unset XDG_DATA_HOME (or set", + " DTVEM_ROOT to point at it) and re-run `dtvem init` so the active install matches.", + "Always restart your terminal after consolidating so PATH picks up the change.", + }, "\n") +} + +func init() { + Register(newXDGSplitCheck()) +} diff --git a/src/internal/doctor/check_xdg_split_test.go b/src/internal/doctor/check_xdg_split_test.go new file mode 100644 index 0000000..abd1c56 --- /dev/null +++ b/src/internal/doctor/check_xdg_split_test.go @@ -0,0 +1,150 @@ +package doctor + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// xdgInstall plants a dtvem-shaped versions tree under root so the +// rootHasData heuristic returns true. Tests use this to set up the +// "split state" condition without writing real runtime binaries. +func xdgInstall(t *testing.T, root string) { + t.Helper() + versionDir := filepath.Join(root, "versions", "python", "3.11.0") + if err := os.MkdirAll(versionDir, 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } +} + +func newXDGSplitCheckWith(active string, candidates []string) *xdgSplitCheck { + c := newXDGSplitCheck() + c.activeRoot = func() string { return active } + c.candidateRoots = func() []string { return candidates } + // Defer to the real hasData against the filesystem so tests + // exercise the rootHasData heuristic too rather than mocking it + // away. + return c +} + +func TestXDGSplitCheck_SingleInstallIsOK(t *testing.T) { + root := t.TempDir() + xdgInstall(t, root) + got := newXDGSplitCheckWith(root, []string{root}).Run() + if !got.OK { + t.Errorf("expected OK with a single install location, got %#v", got) + } +} + +func TestXDGSplitCheck_OrphanInstallIsFlagged(t *testing.T) { + active := t.TempDir() + xdgInstall(t, active) + orphan := t.TempDir() + xdgInstall(t, orphan) + + got := newXDGSplitCheckWith(active, []string{active, orphan}).Run() + if got.OK { + t.Fatalf("expected non-OK with split state, got OK") + } + if got.Severity != SeverityWarning { + t.Errorf("severity: got %s, want warning", got.Severity) + } + if got.Fixable() { + t.Errorf("xdg-split check should be manual, but Finding.Fix is set") + } + + combined := strings.Join(detailValues(got.Details), " ") + if !strings.Contains(combined, orphan) { + t.Errorf("expected orphan path %q in details, got %#v", orphan, got.Details) + } +} + +func TestXDGSplitCheck_EmptyCandidateDirIsIgnored(t *testing.T) { + // A candidate root with no versions// tree is + // not "data" — that's a freshly-initialized but empty install. + // We shouldn't flag it as orphaned. + active := t.TempDir() + xdgInstall(t, active) + emptyOrphan := t.TempDir() + // no xdgInstall call → no version subdirs + + got := newXDGSplitCheckWith(active, []string{active, emptyOrphan}).Run() + if !got.OK { + t.Errorf("expected OK when secondary location is empty, got %#v", got) + } +} + +func TestXDGSplitCheck_ActiveLocationDuplicateIsCollapsed(t *testing.T) { + // candidateRoots may legitimately list the active root more than + // once (e.g., DTVEM_ROOT and XDG_DATA_HOME pointing at the same + // place). That shouldn't be flagged as a split. + active := t.TempDir() + xdgInstall(t, active) + got := newXDGSplitCheckWith(active, []string{active, active, active}).Run() + if !got.OK { + t.Errorf("expected OK when candidates dedupe to active root, got %#v", got) + } +} + +func TestRootHasData_RequiresVersionDir(t *testing.T) { + // Skeleton with no version subdirectory should NOT count as data. + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "versions", "python"), 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if rootHasData(root) { + t.Errorf("rootHasData should return false when no version subdir exists") + } + + // Adding a version subdir flips it. + if err := os.MkdirAll(filepath.Join(root, "versions", "python", "3.11.0"), 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if !rootHasData(root) { + t.Errorf("rootHasData should return true once a version subdir exists") + } +} + +func TestRootHasData_MissingRootIsFalse(t *testing.T) { + if rootHasData(filepath.Join(t.TempDir(), "does-not-exist")) { + t.Errorf("rootHasData should return false for a missing root") + } +} + +func TestDedupePaths_RemovesExactDuplicates(t *testing.T) { + in := []string{"/a", "/b", "/a", "/c"} + got := dedupePaths(in) + want := []string{"/a", "/b", "/c"} + if !stringSliceEqualLocal(got, want) { + t.Errorf("dedupePaths(%v) = %v, want %v", in, got, want) + } +} + +func TestXDGSplitCheck_Registered(t *testing.T) { + found := false + for _, c := range All() { + if c.Name() == "xdg-split-state" { + found = true + break + } + } + if !found { + t.Errorf("xdg-split-state check is not in the default registry") + } +} + +// stringSliceEqualLocal is a copy of the unexported helper used by +// other test files in this package. Inlined to avoid widening the +// public API surface of the package just for tests. +func stringSliceEqualLocal(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/src/internal/doctor/doctor.go b/src/internal/doctor/doctor.go new file mode 100644 index 0000000..6f8336a --- /dev/null +++ b/src/internal/doctor/doctor.go @@ -0,0 +1,108 @@ +// Package doctor diagnoses dtvem configuration issues and, when asked, +// applies fixes for the subset of findings that can be safely remediated +// in-place. +// +// The package is organized around a small Check interface. Each check +// inspects one aspect of the environment (PATH, shims directory, config +// files) and returns a Finding describing what it saw. The doctor command +// in src/cmd/doctor.go wires those findings into a report and, with --fix, +// invokes the per-finding Fix closure where one was supplied. +// +// Checks register themselves into the package-level registry via init(), +// the same pattern used by runtime and migration providers, so adding a +// new check is a self-contained file: write the Check implementation and +// add a Register() call. +package doctor + +// Severity is the priority of a Finding. Severities only carry meaning +// when Finding.OK is false — a passing check has no severity. +type Severity int + +const ( + // SeverityInfo describes a non-problem worth surfacing (e.g., + // "you're on the User PATH install path, just FYI"). + SeverityInfo Severity = iota + // SeverityWarning describes a misconfiguration that doesn't currently + // break anything but is likely to bite the user (e.g., a system + // runtime taking precedence over a dtvem shim). + SeverityWarning + // SeverityError describes a misconfiguration that is actively breaking + // dtvem. The doctor command exits non-zero when any error-severity + // finding is present. + SeverityError +) + +// String returns a stable lowercase identifier for the severity. +func (s Severity) String() string { + switch s { + case SeverityInfo: + return "info" + case SeverityWarning: + return "warning" + case SeverityError: + return "error" + default: + return "unknown" + } +} + +// Detail is a single key/value pair attached to a Finding, used to +// surface the specific data behind the finding (e.g., "Found:" / the +// stale path the check actually saw). Multiple Details are rendered as +// an aligned column under the title. +type Detail struct { + Key string + Value string +} + +// Finding is the result of running a single check. +// +// A check that found nothing wrong returns OK=true; callers should treat +// OK findings as "all green" regardless of severity. The Resolution +// string is informational text shown to the user — either a one-line +// description of what Fix() will do (for fixable findings) or the +// manual remediation steps (for findings without a Fix). +type Finding struct { + // Severity is meaningful only when OK is false. + Severity Severity + + // OK is true when the check passed and no problem was detected. + OK bool + + // Title is a short, human-readable description of the check or the + // problem. Always populated. + Title string + + // Details is an ordered list of key/value pairs to render under the + // title, in the order added. Empty for passing checks. + Details []Detail + + // Resolution describes the remediation. For fixable findings this + // is a one-line summary of what Fix() will do; for manual findings + // it is the step-by-step instructions for the user. + Resolution string + + // Fix, when non-nil, applies an automatic remediation for this + // finding. Checks that require manual intervention leave this nil. + Fix func() error +} + +// Fixable reports whether the finding can be remediated automatically. +func (f Finding) Fixable() bool { + return f.Fix != nil +} + +// Check is the interface every doctor diagnostic implements. +// +// Run() must be side-effect-free: it inspects state and returns a +// Finding. Any state mutation lives in Finding.Fix, which the doctor +// command invokes only when the user has opted in via --fix. +type Check interface { + // Name returns a stable identifier for this check (e.g. + // "stale-shims-path"). Used in verbose output and tests. + Name() string + + // Run executes the check and returns a Finding describing what it + // observed. Run must not modify any state. + Run() Finding +} diff --git a/src/internal/doctor/doctor_test.go b/src/internal/doctor/doctor_test.go new file mode 100644 index 0000000..5bc7481 --- /dev/null +++ b/src/internal/doctor/doctor_test.go @@ -0,0 +1,225 @@ +package doctor + +import ( + "errors" + "testing" +) + +// fakeCheck is a Check implementation backed by closures, used to drive +// the registry and runner tests without standing up real environment +// state. +type fakeCheck struct { + name string + run func() Finding +} + +func (f *fakeCheck) Name() string { return f.name } +func (f *fakeCheck) Run() Finding { return f.run() } + +func okFinding(title string) Finding { + return Finding{OK: true, Title: title} +} + +func errFinding(title string, fix func() error) Finding { + return Finding{ + Severity: SeverityError, + Title: title, + Resolution: "fix it", + Fix: fix, + } +} + +func TestSeverity_String(t *testing.T) { + tests := []struct { + sev Severity + want string + }{ + {SeverityInfo, "info"}, + {SeverityWarning, "warning"}, + {SeverityError, "error"}, + {Severity(99), "unknown"}, + } + for _, tc := range tests { + if got := tc.sev.String(); got != tc.want { + t.Errorf("Severity(%d).String() = %q, want %q", tc.sev, got, tc.want) + } + } +} + +func TestFinding_Fixable(t *testing.T) { + manual := Finding{} + if manual.Fixable() { + t.Error("Finding with nil Fix should not be Fixable()") + } + + auto := Finding{Fix: func() error { return nil }} + if !auto.Fixable() { + t.Error("Finding with non-nil Fix should be Fixable()") + } +} + +func TestRegistry_RegistersAndReturnsCopy(t *testing.T) { + r := NewRegistry() + c1 := &fakeCheck{name: "a"} + c2 := &fakeCheck{name: "b"} + r.Register(c1) + r.Register(c2) + + all := r.All() + if len(all) != 2 { + t.Fatalf("All() returned %d checks, want 2", len(all)) + } + if all[0].Name() != "a" || all[1].Name() != "b" { + t.Errorf("All() preserved order: got [%s, %s], want [a, b]", all[0].Name(), all[1].Name()) + } + + // Mutating the returned slice must not affect the registry's + // internal state. + all[0] = &fakeCheck{name: "mutated"} + again := r.All() + if again[0].Name() != "a" { + t.Errorf("registry leaked internal slice: post-mutation got %q, want %q", again[0].Name(), "a") + } +} + +func TestRegistry_Reset(t *testing.T) { + r := NewRegistry() + r.Register(&fakeCheck{name: "a"}) + r.Reset() + if got := r.All(); len(got) != 0 { + t.Errorf("Reset() did not clear registry: got %d checks", len(got)) + } +} + +func TestRun_PreservesOrderAndPairing(t *testing.T) { + a := &fakeCheck{name: "a", run: func() Finding { return okFinding("a-title") }} + b := &fakeCheck{name: "b", run: func() Finding { return errFinding("b-title", nil) }} + + res := Run([]Check{a, b}) + if len(res.Results) != 2 { + t.Fatalf("Run returned %d results, want 2", len(res.Results)) + } + if res.Results[0].Check.Name() != "a" || res.Results[0].Finding.Title != "a-title" { + t.Errorf("result 0 mispaired: %#v", res.Results[0]) + } + if res.Results[1].Check.Name() != "b" || res.Results[1].Finding.Title != "b-title" { + t.Errorf("result 1 mispaired: %#v", res.Results[1]) + } +} + +func TestResult_HasErrors(t *testing.T) { + tests := []struct { + name string + res Result + want bool + }{ + { + name: "no findings", + res: Result{}, + want: false, + }, + { + name: "all OK", + res: Result{Results: []CheckResult{ + {Finding: okFinding("a")}, + {Finding: okFinding("b")}, + }}, + want: false, + }, + { + name: "warning only", + res: Result{Results: []CheckResult{ + {Finding: Finding{Severity: SeverityWarning, Title: "w"}}, + }}, + want: false, + }, + { + name: "OK error is ignored (OK=true)", + res: Result{Results: []CheckResult{ + // A passing check should never cause HasErrors to fire, + // even if Severity is somehow Error. + {Finding: Finding{OK: true, Severity: SeverityError, Title: "ok-but-error-sev"}}, + }}, + want: false, + }, + { + name: "actual error", + res: Result{Results: []CheckResult{ + {Finding: errFinding("real error", nil)}, + }}, + want: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := tc.res.HasErrors(); got != tc.want { + t.Errorf("HasErrors() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestResult_Fixable(t *testing.T) { + fixerCalled := false + fix := func() error { fixerCalled = true; return nil } + + res := Result{Results: []CheckResult{ + {Check: &fakeCheck{name: "ok"}, Finding: okFinding("passing")}, + {Check: &fakeCheck{name: "manual"}, Finding: errFinding("manual", nil)}, + {Check: &fakeCheck{name: "auto"}, Finding: errFinding("auto", fix)}, + }} + + fixable := res.Fixable() + if len(fixable) != 1 { + t.Fatalf("Fixable() returned %d, want 1 (only non-OK with Fix)", len(fixable)) + } + if fixable[0].Check.Name() != "auto" { + t.Errorf("Fixable() returned wrong check: %q, want %q", fixable[0].Check.Name(), "auto") + } + + // The Fix closure should not have been invoked just by listing — + // remediation is opt-in via the caller, not the runner. + if fixerCalled { + t.Error("Fixable() must not invoke Fix") + } + + // Verify the returned closure is still callable. + if err := fixable[0].Finding.Fix(); err != nil { + t.Errorf("Fix() returned error: %v", err) + } + if !fixerCalled { + t.Error("Fix() did not execute the underlying closure") + } +} + +func TestResult_Fixable_PropagatesErrors(t *testing.T) { + // Sanity check that the Finding.Fix surface returns errors verbatim — + // the doctor command relies on this to report fix failures to users. + want := errors.New("could not write registry") + res := Result{Results: []CheckResult{ + {Finding: errFinding("bad", func() error { return want })}, + }} + got := res.Fixable()[0].Finding.Fix() + if !errors.Is(got, want) { + t.Errorf("Fix() returned %v, want %v", got, want) + } +} + +func TestPackageLevelRegistry_RegisterAndReset(t *testing.T) { + // Snapshot existing registrations so we don't disturb checks + // registered by other packages' init() functions. + saved := All() + Reset() + defer func() { + Reset() + for _, c := range saved { + Register(c) + } + }() + + Register(&fakeCheck{name: "x"}) + got := All() + if len(got) != 1 || got[0].Name() != "x" { + t.Errorf("package-level Register/All round-trip failed: got %#v", got) + } +} diff --git a/src/internal/doctor/registry.go b/src/internal/doctor/registry.go new file mode 100644 index 0000000..6514193 --- /dev/null +++ b/src/internal/doctor/registry.go @@ -0,0 +1,66 @@ +package doctor + +import "sync" + +// Registry holds an ordered list of registered checks. Registration order +// is preserved so the doctor command produces a stable, predictable +// report (alphabetical/severity sorting happens at render time, not here). +type Registry struct { + mu sync.RWMutex + checks []Check +} + +// NewRegistry returns an empty Registry. Most callers should use the +// package-level Register / All instead; NewRegistry is exposed so tests +// can build an isolated registry without touching package state. +func NewRegistry() *Registry { + return &Registry{} +} + +// Register appends a check to the registry. Duplicate names are allowed +// at the type level (the registry is a list, not a map) but discouraged; +// tests should call Reset before registering to keep ordering deterministic. +func (r *Registry) Register(c Check) { + r.mu.Lock() + defer r.mu.Unlock() + r.checks = append(r.checks, c) +} + +// All returns the registered checks in registration order. The returned +// slice is a copy, so callers can sort or filter without affecting the +// registry. +func (r *Registry) All() []Check { + r.mu.RLock() + defer r.mu.RUnlock() + out := make([]Check, len(r.checks)) + copy(out, r.checks) + return out +} + +// Reset clears the registry. Primarily useful in tests that want to +// drive a known set of checks. +func (r *Registry) Reset() { + r.mu.Lock() + defer r.mu.Unlock() + r.checks = nil +} + +var defaultRegistry = NewRegistry() + +// Register adds a check to the package-level registry. Checks call this +// from init() so the doctor command discovers them without any explicit +// wiring at startup. +func Register(c Check) { + defaultRegistry.Register(c) +} + +// All returns every check registered in the package-level registry, in +// registration order. +func All() []Check { + return defaultRegistry.All() +} + +// Reset clears the package-level registry. Tests only. +func Reset() { + defaultRegistry.Reset() +} diff --git a/src/internal/doctor/run.go b/src/internal/doctor/run.go new file mode 100644 index 0000000..5cd22eb --- /dev/null +++ b/src/internal/doctor/run.go @@ -0,0 +1,60 @@ +package doctor + +// CheckResult pairs a check with the finding it produced. The doctor +// command iterates a []CheckResult to render the report; tests use it +// to assert on the structured output rather than parsing rendered text. +type CheckResult struct { + Check Check + Finding Finding +} + +// Result is the aggregated outcome of running every registered check. +type Result struct { + // Results is one entry per check, in the order the checks ran. + Results []CheckResult +} + +// HasErrors reports whether any non-OK finding has SeverityError. The +// doctor command uses this to decide its process exit code. +func (r Result) HasErrors() bool { + for _, cr := range r.Results { + if !cr.Finding.OK && cr.Finding.Severity == SeverityError { + return true + } + } + return false +} + +// Fixable returns the subset of results that have an automatic fix +// available and have not passed. Used by the doctor command to drive +// the per-finding remediation prompts. +func (r Result) Fixable() []CheckResult { + var out []CheckResult + for _, cr := range r.Results { + if !cr.Finding.OK && cr.Finding.Fixable() { + out = append(out, cr) + } + } + return out +} + +// Run executes every check in the given slice and returns the aggregated +// Result. Checks run sequentially in the order provided; we deliberately +// avoid parallelism because several checks read shared environment state +// (PATH, registry, files under ~/.dtvem) and a stable order keeps the +// rendered report easy to read. +func Run(checks []Check) Result { + res := Result{Results: make([]CheckResult, 0, len(checks))} + for _, c := range checks { + res.Results = append(res.Results, CheckResult{ + Check: c, + Finding: c.Run(), + }) + } + return res +} + +// RunAll is shorthand for Run(All()). +func RunAll() Result { + return Run(All()) +} diff --git a/src/internal/doctor/strings.go b/src/internal/doctor/strings.go new file mode 100644 index 0000000..147c376 --- /dev/null +++ b/src/internal/doctor/strings.go @@ -0,0 +1,11 @@ +package doctor + +// plural returns one or many suffix forms based on count. Inline +// helper so the doctor checks don't pull in a heavier pluralization +// dependency for what amounts to a handful of titles per report. +func plural(n int, one, many string) string { + if n == 1 { + return one + } + return many +} diff --git a/src/internal/path/path.go b/src/internal/path/path.go index a97e10a..210a130 100644 --- a/src/internal/path/path.go +++ b/src/internal/path/path.go @@ -2,6 +2,7 @@ package path import ( + "fmt" "os" "path/filepath" "runtime" @@ -10,6 +11,12 @@ import ( "github.com/CodingWithCalvin/dtvem.cli/src/internal/constants" ) +// dtvemMarkerComment is the literal comment line dtvem writes above +// every PATH export it adds to a Unix shell config. The stale-shims +// fix removes a PATH export only when this exact marker is on the +// line above, so we never touch lines the user wrote themselves. +const dtvemMarkerComment = "# Added by dtvem" + // IsInPath checks if a directory is in the system PATH func IsInPath(dir string) bool { pathEnv := os.Getenv("PATH") @@ -71,33 +78,195 @@ func IsDtvemShimsPath(path string) bool { // // Comparison against currentShimsDir is case-insensitive on Windows. func FindStaleShimsEntries(pathEntries []string, currentShimsDir string) []string { + _, stale := PartitionStaleShimsEntries(pathEntries, currentShimsDir) + return stale +} + +// PartitionStaleShimsEntries splits pathEntries into the entries to keep +// and the stale dtvem-shims entries to remove. Order is preserved in +// both slices, and the strings are the original (un-cleaned) entries so +// callers can match them against registry-stored values verbatim. +// +// An empty string entry is treated as "drop silently" — it lands in +// neither slice, which matches what callers want when rewriting PATH +// (we don't want to preserve stray empty entries that arose from +// trailing separators). +// +// When currentShimsDir is empty the entire input is returned as kept, +// since without a "current" reference we can't decide which dtvem-shims +// entries are stale and which are correct. +func PartitionStaleShimsEntries(pathEntries []string, currentShimsDir string) (kept, stale []string) { if currentShimsDir == "" { - return nil + kept = make([]string, 0, len(pathEntries)) + for _, entry := range pathEntries { + if strings.TrimSpace(entry) == "" { + continue + } + kept = append(kept, entry) + } + return kept, nil } currentClean := filepath.Clean(currentShimsDir) - var stale []string for _, entry := range pathEntries { trimmed := strings.TrimSpace(entry) if trimmed == "" { continue } if !IsDtvemShimsPath(trimmed) { + kept = append(kept, entry) continue } entryClean := filepath.Clean(trimmed) + matches := entryClean == currentClean if runtime.GOOS == constants.OSWindows { - if strings.EqualFold(entryClean, currentClean) { - continue - } - } else { - if entryClean == currentClean { - continue - } + matches = strings.EqualFold(entryClean, currentClean) + } + if matches { + kept = append(kept, entry) + continue } stale = append(stale, entry) } - return stale + return kept, stale +} + +// RemoveStaleShimsFromShellConfig removes "marker + export" blocks +// from a single shell config file when the export references a dtvem +// shims directory that doesn't match currentShimsDir. The file is +// rewritten in place; the returned slice lists the stale paths whose +// blocks were dropped, in source order. +// +// Only pairs of lines where the first line is exactly the dtvem +// marker comment are touched. That keeps the fix safe against user- +// edited configs: a PATH export the user wrote themselves won't have +// the marker above it and will be left alone even if it happens to +// reference an old dtvem path. +// +// If the file doesn't exist or contains nothing to remove, the file +// is left alone and the returned slice is empty. The function is +// safe to call on Windows but won't normally find anything there +// because the Windows install path stores PATH in the registry, not +// in a config file — the doctor command's Windows fix routes through +// RemoveStaleShimsFromUserPath instead. +func RemoveStaleShimsFromShellConfig(configFile, currentShimsDir string) ([]string, error) { + data, err := os.ReadFile(configFile) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, fmt.Errorf("read %s: %w", configFile, err) + } + + newContent, removed := removeDtvemMarkerBlocks(string(data), currentShimsDir) + if len(removed) == 0 { + return nil, nil + } + + if err := os.WriteFile(configFile, []byte(newContent), 0644); err != nil { + return nil, fmt.Errorf("write %s: %w", configFile, err) + } + return removed, nil +} + +// removeDtvemMarkerBlocks scans configContent line by line, dropping +// each pair of (marker comment, following export line) whose export +// line references a dtvem shims path that doesn't match +// currentShimsDir. Returns the rewritten content and the stale paths +// that were dropped, in source order. +// +// Behavior intentionally errs on the side of keeping content: +// - A marker comment that isn't followed by anything is preserved. +// - A marker comment followed by an export that doesn't look like a +// dtvem shims path is preserved (some user wrote something +// unrelated under our marker). +// - A marker comment followed by an export pointing at the current +// shims dir is preserved (it's not stale). +func removeDtvemMarkerBlocks(configContent, currentShimsDir string) (string, []string) { + lines := strings.Split(configContent, "\n") + out := make([]string, 0, len(lines)) + var removed []string + + for i := 0; i < len(lines); i++ { + line := lines[i] + if strings.TrimSpace(line) != dtvemMarkerComment || i+1 >= len(lines) { + out = append(out, line) + continue + } + + next := lines[i+1] + stale := extractStaleShimsPath(next, currentShimsDir) + if stale == "" { + out = append(out, line) + continue + } + + // Drop both the marker and the export, recording what we + // removed so the caller can report it to the user. + removed = append(removed, stale) + i++ + } + + return strings.Join(out, "\n"), removed +} + +// extractStaleShimsPath returns the dtvem shims path referenced in a +// PATH-export line if and only if that path is non-empty, looks like a +// dtvem shims dir, and is not currentShimsDir. Returns "" when the +// line isn't a stale-referencing export. +// +// The matcher is permissive across bash/zsh ("export PATH=...") and +// fish ("set -gx PATH ...") because the dtvem shims path appears +// inside double quotes in both cases. We pull every quoted substring, +// split each on the path separator to drop the trailing $PATH the +// bash export concatenates, and check the first segment against +// IsDtvemShimsPath. +func extractStaleShimsPath(line, currentShimsDir string) string { + currentClean := filepath.Clean(currentShimsDir) + + for _, quoted := range quotedSubstrings(line) { + // Bash-style ":$PATH" — keep the part before the first + // colon. Fish-style "" — the whole quoted string is the + // path, so the no-colon case is a no-op. + candidate := quoted + if idx := strings.Index(candidate, ":"); idx >= 0 { + candidate = candidate[:idx] + } + candidate = strings.TrimSpace(candidate) + if !IsDtvemShimsPath(candidate) { + continue + } + if filepath.Clean(candidate) == currentClean { + // References the current shims dir — that's the active + // export we want to leave in place. + continue + } + return candidate + } + return "" +} + +// quotedSubstrings returns every substring of s that is enclosed in +// double quotes, in source order. We deliberately only match "..." +// (not '...' or backticks) because every dtvem-written PATH export +// uses double quotes; accepting other styles would broaden the match +// surface for user-written lines we don't want to touch. +func quotedSubstrings(s string) []string { + var out []string + i := 0 + for { + start := strings.IndexByte(s[i:], '"') + if start < 0 { + return out + } + start += i + 1 + end := strings.IndexByte(s[start:], '"') + if end < 0 { + return out + } + out = append(out, s[start:start+end]) + i = start + end + 1 + } } // SplitPath splits the PATH environment variable using the OS-appropriate separator. diff --git a/src/internal/path/path_test.go b/src/internal/path/path_test.go index 31e0b35..1d50bb8 100644 --- a/src/internal/path/path_test.go +++ b/src/internal/path/path_test.go @@ -581,3 +581,256 @@ func TestFindExecutableInDir(t *testing.T) { } }) } + +func TestPartitionStaleShimsEntries(t *testing.T) { + // Use the platform-native separators so the case-insensitive + // comparison on Windows is exercised against realistic strings. + current := filepath.Join("C:", "Users", "u", ".local", "share", "dtvem", "shims") + stale1 := filepath.Join("C:", "Users", "u", ".dtvem", "shims") + stale2 := filepath.Join("C:", "Users", "u", "old", "dtvem", "shims") + unrelated := filepath.Join("C:", "Windows", "System32") + + tests := []struct { + name string + entries []string + current string + wantKept []string + wantRemoved []string + }{ + { + name: "current shims dir is kept, stale ones removed", + entries: []string{current, stale1, unrelated, stale2}, + current: current, + wantKept: []string{current, unrelated}, + wantRemoved: []string{stale1, stale2}, + }, + { + name: "empty entries are dropped from kept too", + entries: []string{"", current, " ", stale1}, + current: current, + wantKept: []string{current}, + wantRemoved: []string{stale1}, + }, + { + name: "empty current returns everything as kept", + entries: []string{current, stale1, unrelated}, + current: "", + wantKept: []string{current, stale1, unrelated}, + wantRemoved: nil, + }, + { + name: "no entries means no kept and no removed", + entries: nil, + current: current, + wantKept: nil, + wantRemoved: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + kept, removed := PartitionStaleShimsEntries(tt.entries, tt.current) + if !stringSliceEqual(kept, tt.wantKept) { + t.Errorf("kept = %v, want %v", kept, tt.wantKept) + } + if !stringSliceEqual(removed, tt.wantRemoved) { + t.Errorf("removed = %v, want %v", removed, tt.wantRemoved) + } + }) + } +} + +func TestRemoveDtvemMarkerBlocks(t *testing.T) { + current := "/home/u/.local/share/dtvem/shims" + + tests := []struct { + name string + input string + current string + wantOut string + wantRemoved []string + }{ + { + name: "bash export with stale path is removed", + input: `# some user line +# Added by dtvem +export PATH="/home/u/.dtvem/shims:$PATH" +# another user line`, + current: current, + wantRemoved: []string{"/home/u/.dtvem/shims"}, + wantOut: `# some user line +# another user line`, + }, + { + name: "fish set -gx with stale path is removed", + input: `# Added by dtvem +set -gx PATH "/home/u/.dtvem/shims" $PATH`, + current: current, + wantRemoved: []string{"/home/u/.dtvem/shims"}, + wantOut: ``, + }, + { + name: "current shims dir is kept (not stale)", + input: `# Added by dtvem +export PATH="/home/u/.local/share/dtvem/shims:$PATH"`, + current: current, + wantOut: `# Added by dtvem +export PATH="/home/u/.local/share/dtvem/shims:$PATH"`, + wantRemoved: nil, + }, + { + name: "user-written export without marker is preserved", + input: `export PATH="/home/u/.dtvem/shims:$PATH" +# Added by dtvem +export PATH="/home/u/.dtvem/shims:$PATH"`, + current: current, + wantRemoved: []string{"/home/u/.dtvem/shims"}, + wantOut: `export PATH="/home/u/.dtvem/shims:$PATH"`, + }, + { + name: "marker with no following line is preserved", + input: `# Added by dtvem`, + current: current, + wantOut: `# Added by dtvem`, + wantRemoved: nil, + }, + { + name: "multiple stale blocks are all removed", + input: `# Added by dtvem +export PATH="/home/u/.dtvem/shims:$PATH" +some_user_content=1 +# Added by dtvem +export PATH="/home/u/old/dtvem/shims:$PATH"`, + current: current, + wantRemoved: []string{"/home/u/.dtvem/shims", "/home/u/old/dtvem/shims"}, + wantOut: `some_user_content=1`, + }, + { + name: "marker followed by non-dtvem export is preserved", + input: `# Added by dtvem +echo hello`, + current: current, + wantOut: `# Added by dtvem +echo hello`, + wantRemoved: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotOut, gotRemoved := removeDtvemMarkerBlocks(tt.input, tt.current) + if gotOut != tt.wantOut { + t.Errorf("out = %q\nwant %q", gotOut, tt.wantOut) + } + if !stringSliceEqual(gotRemoved, tt.wantRemoved) { + t.Errorf("removed = %v, want %v", gotRemoved, tt.wantRemoved) + } + }) + } +} + +func TestRemoveStaleShimsFromShellConfig_MissingFileIsNoop(t *testing.T) { + // Calling on a non-existent file should be a silent no-op, since + // "this user doesn't have a config file at all" is a valid state + // and not a problem doctor should escalate. + removed, err := RemoveStaleShimsFromShellConfig(filepath.Join(t.TempDir(), "no-such-file"), "/anything") + if err != nil { + t.Errorf("expected nil error for missing file, got %v", err) + } + if len(removed) != 0 { + t.Errorf("expected no removed entries, got %v", removed) + } +} + +func TestRemoveStaleShimsFromShellConfig_RewritesFile(t *testing.T) { + dir := t.TempDir() + configFile := filepath.Join(dir, ".bashrc") + content := `# user line +# Added by dtvem +export PATH="/home/u/.dtvem/shims:$PATH" +# another user line +` + if err := os.WriteFile(configFile, []byte(content), 0644); err != nil { + t.Fatalf("write: %v", err) + } + + removed, err := RemoveStaleShimsFromShellConfig(configFile, "/home/u/.local/share/dtvem/shims") + if err != nil { + t.Fatalf("RemoveStaleShimsFromShellConfig: %v", err) + } + if len(removed) != 1 || removed[0] != "/home/u/.dtvem/shims" { + t.Errorf("removed: got %v, want [/home/u/.dtvem/shims]", removed) + } + + got, err := os.ReadFile(configFile) + if err != nil { + t.Fatalf("read: %v", err) + } + want := `# user line +# another user line +` + if string(got) != want { + t.Errorf("rewritten file:\n got %q\nwant %q", got, want) + } +} + +func TestRemoveStaleShimsFromShellConfig_LeavesFileAloneWhenNothingToDo(t *testing.T) { + dir := t.TempDir() + configFile := filepath.Join(dir, ".bashrc") + original := `# user line only` + if err := os.WriteFile(configFile, []byte(original), 0644); err != nil { + t.Fatalf("write: %v", err) + } + // Read mtime so we can assert we didn't rewrite the file. + before, _ := os.Stat(configFile) + + removed, err := RemoveStaleShimsFromShellConfig(configFile, "/home/u/.local/share/dtvem/shims") + if err != nil { + t.Fatalf("RemoveStaleShimsFromShellConfig: %v", err) + } + if len(removed) != 0 { + t.Errorf("expected no removed entries, got %v", removed) + } + after, _ := os.Stat(configFile) + // On some filesystems mtime resolution is coarse, so equality is + // the right check rather than "after >= before". + if !before.ModTime().Equal(after.ModTime()) { + t.Errorf("file was rewritten when there was nothing to remove (mtime changed)") + } +} + +func TestQuotedSubstrings(t *testing.T) { + tests := []struct { + in string + want []string + }{ + {`export PATH="/a:$PATH"`, []string{`/a:$PATH`}}, + {`set -gx PATH "/a" $PATH`, []string{`/a`}}, + {`a "b" c "d"`, []string{`b`, `d`}}, + {`no quotes here`, nil}, + {`unterminated "ohno`, nil}, + {`empty "" here`, []string{``}}, + } + for _, tt := range tests { + got := quotedSubstrings(tt.in) + if !stringSliceEqual(got, tt.want) { + t.Errorf("quotedSubstrings(%q) = %v, want %v", tt.in, got, tt.want) + } + } +} + +// stringSliceEqual returns true when a and b have the same length and +// identical elements. Defined locally because reflect.DeepEqual treats +// nil and []string{} as different and that distinction is noise in +// these tests. +func stringSliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/src/internal/path/path_windows.go b/src/internal/path/path_windows.go index c6334e5..3556e15 100644 --- a/src/internal/path/path_windows.go +++ b/src/internal/path/path_windows.go @@ -618,3 +618,53 @@ func IsSetxAvailable() bool { _, err := exec.LookPath("setx") return err == nil } + +// RemoveStaleShimsFromUserPath removes any stale dtvem shims entries +// from the current user's PATH registry value, preserving every other +// entry (and the current shims dir itself if present) in their original +// order. Returns the list of entries that were actually removed. +// +// This is the surgical counterpart to modifyUserPath, which is scoped +// to the "set up dtvem from scratch" flow and bundles removal with +// re-ordering plus re-adding the current shims dir at the front. The +// doctor command needs a targeted "just delete the stale entries" +// operation, and re-using modifyUserPath would re-prioritize PATH in +// ways the user didn't ask for. +// +// Operates on User PATH only. Stale entries in System PATH require +// administrator privileges and aren't touched here; doctor surfaces +// that case separately in its resolution text. +func RemoveStaleShimsFromUserPath(currentShimsDir string) ([]string, error) { + key, err := registry.OpenKey(registry.CURRENT_USER, `Environment`, registry.QUERY_VALUE|registry.SET_VALUE) + if err != nil { + return nil, fmt.Errorf("failed to open User PATH registry key: %w", err) + } + defer func() { _ = key.Close() }() + + currentPath, _, err := key.GetStringValue("Path") + if err != nil && !errors.Is(err, registry.ErrNotExist) { + return nil, fmt.Errorf("failed to read User PATH: %w", err) + } + if errors.Is(err, registry.ErrNotExist) || currentPath == "" { + // No User PATH set at all means there can't be any stale + // entries to remove. + return nil, nil + } + + entries := strings.Split(currentPath, ";") + kept, removed := PartitionStaleShimsEntries(entries, currentShimsDir) + if len(removed) == 0 { + return nil, nil + } + + if err := key.SetStringValue("Path", strings.Join(kept, ";")); err != nil { + return nil, fmt.Errorf("failed to update User PATH in registry: %w", err) + } + + // Tell the shell that PATH changed so newly-spawned processes pick + // up the new value; running shells still need to be restarted to + // see it, which we surface in the doctor resolution text. + broadcastSettingChange() + + return removed, nil +}