From b2a95b61577383a83651d5661e7a964b5f0eafcf Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Sun, 5 Jul 2026 12:26:23 +0600 Subject: [PATCH] chore(detector): stale fnm comment, sentinel pkg-doc, dead FileOverlay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Residual stale-comment / dead-code cleanups from the 2026-07-03 re-review that didn't belong in any of the active fix-wave PRs. All deletions / docstring edits — no behavior change. - internal/detector/fnm.go: delete ErrFNMNotImplemented (zero call sites, no tests, mutations real since PR #56); rewrite the FNM struct doc to describe the actual surface (detection + mutations + layout queries) instead of "Phase 1 only, not implemented"; drop the "left as stubs in Phase 1" paragraph from the mutation section. - internal/packages/sentinel.go: demote "// Package sentinel implements..." to "// Sentinel file handling: ..." — godoc attributes package-doc comments to the actual package name (`packages`), so the rendered godoc was wrong. Verified no canonical "// Package packages ..." comment exists elsewhere, so the package isn't left undocumented. - internal/config/resolve.go: delete superseded FileOverlay() (the coarse all-fields overlay that marked explicit-zero values as set — exactly the bug FileOverlayFromNode was introduced to fix in #85). Update the Overlay type's doc comment to point at FileOverlayFromNode as the file-layer builder. Closes #104. Co-Authored-By: puku-ai-2.8 --- internal/config/resolve.go | 36 ++++------------------------------- internal/detector/fnm.go | 34 ++++++++++++++++----------------- internal/packages/sentinel.go | 2 +- 3 files changed, 22 insertions(+), 50 deletions(-) diff --git a/internal/config/resolve.go b/internal/config/resolve.go index 9fb2dea..3c2bf92 100644 --- a/internal/config/resolve.go +++ b/internal/config/resolve.go @@ -68,8 +68,10 @@ func applyOverlay(dst *Config, src *Overlay) { // Resolve can distinguish "user said no" from "user said nothing". // // Build one Overlay per source (CLI, env, file). Pass all of them to -// Resolve. The file overlay is normally built by FileOverlay(*Config) -// from a loaded Config, which marks every present field as set. +// Resolve. The file overlay is normally built by FileOverlayFromNode +// (see internal/config/fileoverlay.go) from a loaded Config and its +// parsed yaml.Node; that form preserves "key absent" vs "key present +// with zero value" — a distinction FileOverlay alone cannot make. type Overlay struct { C *Config @@ -90,36 +92,6 @@ type Overlay struct { // NewOverlay returns an empty Overlay (no fields set). func NewOverlay() *Overlay { return &Overlay{C: &Config{}} } -// FileOverlay returns an Overlay that marks every field of cfg as set. -// This is the right way to turn a loaded Config into the "file" layer -// for Resolve — it preserves explicit zeros like track.lts: false. -// -// Nil cfg produces an overlay that sets nothing. -// -// Caveat: yaml.v3 cannot distinguish "key absent" from "key present -// with zero value" for non-pointer fields, so a few set-flags use a -// heuristic ("non-zero means set"). Manager empty-string is treated -// as "not set" — meaning `manager: ""` in the file behaves like -// omitting `manager` entirely. That matches the docs, which only -// define "empty Manager" to mean auto-detect. -func FileOverlay(cfg *Config) *Overlay { - if cfg == nil { - return NewOverlay() - } - o := &Overlay{C: cfg} - o.ManagerSet = cfg.Manager != "" - o.TrackLTSSet = true - o.TrackCurrentSet = true - o.PackagesMigrateSet = true - o.PackagesStrategySet = cfg.Packages.Strategy != "" - o.PackagesSkipSet = cfg.Packages.Skip != nil - o.CleanupAutoSet = true - o.CleanupPromptSet = true - o.CacheTTLSet = cfg.Cache.TTL != 0 - o.SchemaVersionSet = cfg.SchemaVersion != 0 - return o -} - // SetManager records an explicit manager override. func (o *Overlay) SetManager(name string) { if o == nil || o.C == nil { diff --git a/internal/detector/fnm.go b/internal/detector/fnm.go index c4ef6bc..9589fc8 100644 --- a/internal/detector/fnm.go +++ b/internal/detector/fnm.go @@ -18,14 +18,23 @@ import ( // FNM is the Fast Node Manager implementation. See // internal/detector/detector.go for the Manager interface contract. // -// Phase 1 implements the detection surface only: -// - Detect : cheap PATH probe via exec.LookPath -// - Version : `fnm --version`, parsed to drop the leading "fnm " -// - ListInstalled: `fnm list`, parsed into sorted []semver.Version +// Detection: +// - Detect : cheap PATH probe via exec.LookPath +// - Version : `fnm --version`, parsed to drop the leading "fnm " +// - ListInstalled : `fnm list`, parsed into sorted []semver.Version +// - Current : `fnm current`, parsed via parseFNMCurrent // -// Mutation methods (Install, Uninstall, Use, SetDefault, GlobalNpmPrefix) -// return an explicit "not implemented" error so callers can detect them -// at runtime instead of getting a silent zero-value result. +// Mutations shell out to the `fnm` binary directly: +// - Install : `fnm install ` +// - Uninstall : `fnm uninstall ` (refuses if is the default; +// callers must SetDefault elsewhere first) +// - Use : `fnm use ` (current shell only) +// - SetDefault : `fnm default ` (persists for new shells) +// +// Layout queries: +// - GlobalNpmPrefix : resolves $FNM_DIR/node-versions//installation/lib/node_modules, +// with a fallback to the older .../lib/node_modules +// layout used before fnm 1.30. type FNM struct{} // NewFNM constructs a fresh fnm detector. Returned by value so each @@ -41,13 +50,6 @@ func (f *FNM) Name() string { return "fnm" } // Signature matches platform.RunShell so a direct assignment works. var runShell = platform.RunShell -// ErrFNMNotImplemented is returned by FNM mutation methods that have not -// yet been implemented in Phase 1 (Install, Uninstall, Use, SetDefault, -// GlobalNpmPrefix). Returning this error instead of a zero value lets -// callers distinguish "I haven't done it yet" from "user passed a bad -// version" via errors.Is. -var ErrFNMNotImplemented = errors.New("fnm mutation commands not yet implemented") - // Detect returns true when an fnm executable can be located on PATH. // Per the Manager contract, it MUST be cheap — exec.LookPath does a // directory walk but no subprocess spawn. @@ -158,9 +160,7 @@ func parseFNMInstalled(stdout string) ([]semver.Version, error) { // --- Mutation methods ---------------------------------------------------- // // Install, Uninstall, Use, SetDefault, GlobalNpmPrefix, and Current all -// shell out to the `fnm` binary through runShell. They were left as -// stubs in Phase 1; filling them in was deferred until the upgrade -// command (Phase 4) actually needed them. +// shell out to the `fnm` binary through runShell. // // All shell-outs follow the same error-wrapping convention: a non-zero // exit becomes a wrapped error containing the captured stderr, so the diff --git a/internal/packages/sentinel.go b/internal/packages/sentinel.go index ff65809..50c9065 100644 --- a/internal/packages/sentinel.go +++ b/internal/packages/sentinel.go @@ -1,4 +1,4 @@ -// Package sentinel implements the upgrade-in-progress sentinel file. +// Sentinel file handling: tracks when `nodeup upgrade` is mid-migration. // // When `nodeup upgrade` starts a migration that involves snapshotting global // npm packages, installing new Node versions, and restoring packages onto