Tool abstraction layer with :download / :path / :install / :update / :uninstall pattern

[munezaclovis]

Summary

• Introduces a Tool abstraction (internal/tools/) with five standardized commands per managed binary: :download, :path, :install, :update, :uninstall
• All tool binaries now stored in ~/.pv/internal/bin/ (private), exposed to ~/.pv/bin/ (user PATH) via shims or symlinks
• Orchestrator commands (install, update, uninstall) delegate to per-tool RunE functions — no duplicated logic
• pv update self-updates the pv binary via syscall.Exec re-exec before updating tools
• pv uninstall delegates cleanup to each tool's :uninstall, then handles global concerns (daemon, DNS, CA cert)
• Daemon commands migrated to colon-namespace: daemon:enable, daemon:disable, daemon:restart
• pv restart delegates to daemon:restart in daemon mode
• colima:path added for power users to expose colima on PATH
• CLAUDE.md rewritten from documentation to concise rules/conventions
• README expanded with full command reference, services, architecture overview

Changes

Area	What changed
internal/tools/	New package: Tool struct, registry, Expose/Unexpose/IsExposed/ExposeAll
internal/tools/shims.go	PHP + Composer shim scripts (moved from phpenv/shim.go)
internal/selfupdate/	New package: NeedsUpdate, Update for pv binary self-update
cmd/*_download.go	New :download commands for php, mago, composer, colima
cmd/*_path.go	New :path commands with --remove flag
cmd/*_update.go	New :update commands using IsExposed() for re-exposure
cmd/*_uninstall.go	New :uninstall commands with spinner UI
cmd/*_install.go	Refactored to delegate to :download RunE
cmd/update.go	Orchestrator with self-update + syscall.Exec re-exec
cmd/uninstall.go	Orchestrator delegating to per-tool :uninstall
cmd/daemon.go	daemon:enable, daemon:disable, daemon:restart
internal/config/paths.go	ComposerPharPath → InternalBinDir, added MagoPath
internal/phpenv/shim.go	Thin wrapper → tools.ExposeAll()
CLAUDE.md	Rewritten as concise instructions
README.md	Expanded with architecture + command reference

Test plan

• go test ./... passes
• go build -o pv . builds clean
• pv mago:download → binary in ~/.pv/internal/bin/mago
• pv mago:path → symlink in ~/.pv/bin/mago
• pv mago:path --remove → symlink removed
• pv mago:install → download + symlink
• pv install --force → full install with new paths
• pv update --no-self-update → updates tools without self-update
• pv daemon:restart → restarts daemon when loaded
• E2E tests pass on CI

[munezaclovis]

PR Review Summary: feat/tool-abstraction

Scope: 37 files, +1915 / -562 lines across 3 commits

---

Critical Issues (4 found)

1. Systematic _ = tools.Expose() / tools.Unexpose() error swallowing

7 locations silently discard errors from PATH exposure, leaving users with broken shims/symlinks while reporting success. If creating a shim or symlink fails (permission denied, disk full, BinDir missing), the user sees success but gets mysterious "command not found" errors.

Affected locations:

• cmd/php_install.go:57 — _ = tools.Expose(t)
• cmd/php_update.go:44 — _ = tools.Expose(t)
• cmd/php_uninstall.go:21 — _ = tools.Unexpose(t)
• cmd/mago_uninstall.go:19 — _ = tools.Unexpose(t)
• cmd/composer_uninstall.go:19 — _ = tools.Unexpose(t)
• cmd/colima_uninstall.go:35 — _ = tools.Unexpose(t)

Fix: Propagate the error:

if err := tools.Expose(t); err != nil {
    return fmt.Errorf("cannot expose %s: %w", name, err)
}

---

2. colima:uninstall discards Stop/Delete errors

cmd/colima_uninstall.go:24-25:

_ = colima.Stop()
_ = colima.Delete()

If Stop() fails, Delete() may also fail because Colima is still running. Then the binary is removed (line 28), but the Colima VM is still running with no way to control it (binary gone, VM still active consuming CPU/RAM).

Fix:

if colima.IsRunning() {
    if err := colima.Stop(); err != nil {
        return "", fmt.Errorf("cannot stop Colima VM (stop it manually before uninstalling): %w", err)
    }
    if err := colima.Delete(); err != nil {
        return "", fmt.Errorf("cannot delete Colima VM: %w", err)
    }
}

---

3. Orchestrator pv uninstall discards all tool errors

cmd/uninstall.go:81-84:

_ = colimaUninstallCmd.RunE(colimaUninstallCmd, nil)
_ = phpUninstallCmd.RunE(phpUninstallCmd, nil)
_ = magoUninstallCmd.RunE(magoUninstallCmd, nil)
_ = composerUninstallCmd.RunE(composerUninstallCmd, nil)

User sees "pv has been completely uninstalled" even if cleanup partially failed. Zero indication of which tools were actually removed.

Fix: Log each failure visually:

if err := colimaUninstallCmd.RunE(colimaUninstallCmd, nil); err != nil {
    fmt.Fprintf(os.Stderr, "  %s Colima uninstall failed: %v\n", ui.Red.Render("!"), err)
}

---

4. pv update always exits 0

cmd/update.go:34-57: All tool update errors are printed but never returned. The command always returns nil.

Impact: CI/CD pipelines using pv update && deploy won't catch failures.

Fix: Track failures and return a summary error:

var failures []string
// ... collect failures ...
if len(failures) > 0 {
    return fmt.Errorf("some updates failed: %s", strings.Join(failures, "; "))
}

---

Important Issues (10 found)

5. phpenv/shim.go directly imports tools — contradicts documented pattern

internal/phpenv/shim.go:3 directly imports internal/tools, but CLAUDE.md (rewritten in this PR) says: "phpenv and tools cannot import each other. This is resolved via callback."

The ExposeFunc callback pattern exists specifically to break this cycle, but shim.go bypasses it with a direct import. While it compiles today (because tools doesn't import phpenv), it prevents tools from ever importing phpenv and makes the documented pattern partially redundant.

Fix: Move wiring to cmd/ or a separate init package:

// in cmd/ or internal/wiring/:
func init() {
    phpenv.ExposeFunc = func(name string) error {
        t := tools.Get(name)
        if t == nil { return nil }
        return tools.Expose(t)
    }
}

---

6. CLAUDE.md claims install is a thin orchestrator — it isn't

CLAUDE.md:48 states: "install, update, and uninstall are thin orchestrators. They MUST NOT contain download, exposure, or cleanup logic."

But cmd/install.go directly calls phpenv.InstallProgress(), binaries.InstallBinary(), colima.Install(), and tools.ExposeAll() inline. It contains substantial download and exposure logic.

Fix: Either refactor pv install to delegate to per-tool :install RunE functions, or amend CLAUDE.md to note that install is an exception (it predates the abstraction and handles interactive bootstrapping differently).

---

7. os.Remove(linkPath) error ignored before symlink creation

internal/tools/tool.go:126:

os.Remove(linkPath) // remove existing

If removal fails for a reason other than "not exists" (e.g., permission denied, or linkPath is a directory), the subsequent os.Symlink() error message says "cannot create symlink" when the real problem is "cannot remove existing file."

Fix:

if err := os.Remove(linkPath); err != nil && !os.IsNotExist(err) {
    return fmt.Errorf("cannot remove existing %s: %w", linkPath, err)
}

---

8. globalPHPVersion() silently returns "" on settings load failure

internal/tools/tool.go:36-41:

func globalPHPVersion() string {
    s, err := config.LoadSettings()
    if err != nil || s.GlobalPHP == "" {
        return ""
    }
    return s.GlobalPHP
}

If LoadSettings() fails (corrupted JSON, permission error), it silently returns "", causing PhpVersionDir("") to produce a path like ~/.pv/php//php — a nonsensical path. Any symlink created from this will be broken, and the corrupted settings error is completely hidden.

---

9. setup.go discards multiple errors silently

• cmd/setup.go:179, 199 — vs, _ := binaries.LoadVersions() — if version state can't be loaded, version tracking is silently skipped
• cmd/setup.go:189, 209 — _ = vs.Save() — if save fails, version manifest becomes inconsistent
• cmd/setup.go:166 — phpenv.SetGlobal() error is checked but no failure message is shown to the user
• cmd/setup.go:27 — installedVersions, _ := phpenv.InstalledVersions() — treats unreadable directory as "no versions"
• cmd/setup.go:90 — settings, _ := config.LoadSettings() — corrupted settings silently default TLD to "test"

---

10. :update commands inline download logic instead of delegating

The :update commands duplicate download calls instead of delegating to :download RunE, creating maintenance risk where download logic changes need to be applied in two places.

Affected files:

• cmd/colima_update.go:25 — calls colima.Install() directly
• cmd/mago_update.go:42 — calls binaries.InstallBinaryProgress() directly
• cmd/composer_update.go:31 — calls binaries.InstallBinaryProgress() directly

---

11. selfupdate package has zero tests

internal/selfupdate/selfupdate.go contains critical logic — version comparison (NeedsUpdate), atomic binary replacement (Update), platform detection — all untested.

Key testable areas:

• NeedsUpdate with matching/differing versions
• NeedsUpdate with "dev" and "" (never-update cases)
• Version prefix normalization (v1.0.0 vs 1.0.0)
• downloadURL and platformString correctness

A bug here could cause infinite re-exec loops or silently skip updates.

---

12. Inconsistent nil guards on tools.Get() across cmd files

cmd/mago_path.go:17 does NOT check for nil after tools.Get("mago"), but cmd/mago_install.go:23, cmd/mago_update.go:51, and cmd/mago_uninstall.go:17 all DO check. Same inconsistency across other tool command sets.

Fix: Either always guard, or add a tools.MustGet(name) that panics on unknown names (making it clear missing tools are programming errors).

---

13. "Unreachable" comment is wrong

cmd/update.go:38:

return nil // unreachable -- syscall.Exec replaces the process

This line IS reachable when syscall.Exec fails — selfUpdate returns (true, error), the error is printed but not returned, and execution falls through to this return nil.

Fix: Change to: return nil // reached only if syscall.Exec failed (error already printed above)

---

14. Missing error context in selfupdate.go

internal/selfupdate/selfupdate.go:117-118:

body, err := io.ReadAll(resp.Body)
if err != nil {
    return "", err
}

Every other error in the file wraps with context (fmt.Errorf("...: %w", err)), but this one returns a bare I/O error with no indication it came from the GitHub API version check.

---

Suggestions (8 found)

15. Make All map unexported

internal/tools/tool.go:44: var All = map[string]*Tool{...} exposes mutable pointers. Any package can do tools.All["mago"].AutoExpose = false and alter global behavior.

Fix: Rename to var all = ..., keep Get() and List() as the public API.

---

16. Add TestRegistryIntegrity

Add a test that iterates all entries in the registry and validates:

• Map key matches Tool.Name
• InternalPath is non-nil
• ExposureShim implies WriteShim != nil
• DisplayName is non-empty

This catches misconfigurations at test time instead of runtime.

---

17. Add MustGet(name) helper

For cmd/*_path.go files where tool names are compile-time constants, a panicking MustGet makes it clear that missing tools are programming errors, not runtime conditions.

---

18. Extract shared bash logic in shims

internal/tools/shims.go: The resolve_version() bash function (~30 lines) is copy-pasted identically between the PHP and Composer shim scripts. If the resolution algorithm changes, both must be updated in lockstep.

Fix: Extract to a const resolveVersionBash and compose it into both templates.

---

19. Remove no-op switch in platformString()

internal/selfupdate/selfupdate.go:132-138: Maps "amd64"→"amd64" and "arm64"→"arm64" — identity mappings that add confusion. Could be replaced with fmt.Sprintf("%s-%s", runtime.GOOS, runtime.GOARCH).

---

20. Eliminate Name duplication in tool registry

Every entry in All has its Name field matching the map key (e.g., key "mago", field Name: "mago"). If someone changes one but not the other, Unexpose and IsExposed will use the wrong path.

Fix: Either derive Name from map key during registration, or add validation that keys match names.

---

21. Fix step numbering in install.go

cmd/install.go:207: Comment says "Step 5: DNS resolver" but line 189 also says "Step 5: Configure environment". Duplicate step numbers.

---

22. README.md symlink notation

README.md:149: Shows ../php/{ver}/frankenphp using relative path, but the actual code in tool.go:124-129 creates symlinks using absolute paths from t.InternalPath().

---

Test Coverage Gaps

Priority	Gap	Severity	Effort
1	selfupdate.NeedsUpdate version logic	9/10	Low — pure function
2	Expose with nil WriteShim error path	8/10	Low — 5-line test
3	Expose symlink overwrite/re-expose	8/10	Low — extend existing
4	ExposeFunc callback wiring in phpenv	7/10	Medium — needs mock
5	:path command structure tests	7/10	Low — follow pattern
6	ExposeAll error propagation	6/10	Low — inject failing WriteShim
7	Orchestrator delegation verification	6/10	Medium

---

Strengths

• Clean tool abstraction — The 5-command pattern is well-designed and consistently applied across all tools
• Good test isolation — All tests use t.TempDir() and t.Setenv("HOME", ...) per conventions
• selfupdate package encapsulation — Clean 2-function public API with well-structured internals
• InternalPath as a function — Smart design for dynamic path resolution (PHP version-dependent paths)
• Proper UI helper usage — ui.Step, ui.StepProgress, ui.Header/ui.Footer used consistently
• CLAUDE.md rewrite — Comprehensive and mostly accurate project documentation
• All commands use RunE — No use of Run, errors propagate correctly (except the swallowed ones above)
• Correct binary storage — internal/bin/ for real binaries, bin/ for shims/symlinks

---

Recommended Action

1. Fix critical issues 1-4 — Propagate or handle errors from Expose/Unexpose/Stop/Delete. Make pv update return non-zero on failures.
2. Address important issues 5-6 — Fix the import pattern and CLAUDE.md accuracy.
3. Add tests for selfupdate.NeedsUpdate — Pure function, highly testable, high impact.
4. Consider suggestions 15-22 — Registry encapsulation and shim deduplication are low-effort, high-value improvements.