feat(cli): implement end-to-end upgrade command#20
Merged
Conversation
ddc0b39 to
0f82baa
Compare
Owner
Author
Code reviewFound 1 issue:
Lines 21 to 25 in 188b887 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Wires the full upgrade flow: - detect -> resolve -> fetch versions -> diff - snapshot global packages - install new Node.js versions - set default version - restore packages Supports --lts, --current, --dry-run, --no-migrate, --offline flags. Also fixes internal/node/dist.go: ManifestVersion's `lts` field was typed bool, but nodejs.org returns a JSON union (`false` for Current, a string codename for LTS). The struct's json.Unmarshal rejected every LTS entry, making FetchManifest fail in production. Rebuilt the field as *LTSCodename with a custom UnmarshalJSON that handles both shapes, and added a TestManifestUnmarshalUnion round-trip test against the real index.json shape. Closes #14
188b887 to
46ef9f3
Compare
added 2 commits
June 30, 2026 13:28
Per the project invariants recorded in CLAUDE.md, cobra RunE implementations should use cmd.Context() rather than context.Background() so the editor/Pty Host can cancel work in-flight when the user aborts (e.g. an upgrade running in an agent terminal that gets killed). Switches the two snapshot/restore loops in runUpgrade over to cmd.Context(). Also stop swallowing the error from parseVersion when computing the install plan. The previous `v, _ := parseVersion(...)` would nil-deref the moment the manifest contained an unparseable row (an LTS line missing a semver suffix, a stray pre-release tag, etc.). Surface the error and abort the upgrade instead.
Adds two entries under [Unreleased] in CHANGELOG.md: - Added: the now-end-to-end 'nodeup upgrade' command (detect → resolve → fetch → snapshot → install → migrate → cleanup) with --lts, --current, --dry-run, --no-migrate, --manager, --offline flags. - Changed: ManifestVersion's (LTS bool, TS string) pair is replaced with a single LTSCodename *string plus a custom UnmarshalJSON so the nodejs.org 'lts' field union (false for Current, codename string for LTS) decodes cleanly. The old shape silently dropped the codename whenever 'lts' was the JSON literal false, which could route a Current release through LatestLTS().
Adds a Claude Code editor helper at the repo root documenting the project's architecture, build/test commands, code invariants (output routing through internal/ui, cmd.Context() over context.Background(), filepath.Join, platform.RunShell), commit/PR conventions, and the current Phase status table. Out of scope for the upgrade PR; added here for editor convenience. The CHANGELOG note for the upgrade command still applies — this file is purely editor-facing.
19df916 to
def2ab0
Compare
|
✔️ 46ef9f3...def2ab0 - Conventional commits check succeeded. |
This was referenced Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes out Phase 4:
nodeup upgradeis now end-to-end instead of a stub. It detects the active Node version manager, resolves the latest LTS and/or Current from the nodejs.org index, computes an install plan (skipping anything already installed), snapshots the active Node version's global npm packages, installs the new versions, sets the newest one as the default, and restores the package snapshot onto the new versions.--dry-runprints the plan and exits without touching the disk.This PR also fixes a latent JSON-union bug in
internal/node/dist.gothat the new upgrade flow exposed: the nodejs.orgltsfield is either the literalfalse(Current) or a codename string (LTS). The previous(LTS bool, TS string)pair silently dropped the codename wheneverltswas JSONfalse, soLatestLTS/LatestCurrentcould mis-classify rows.ManifestVersionis now(LTSCodename *string)with a customUnmarshalJSON; tests cover both shapes.Linked issues
Refs PR #20 review (Phase 4 — upgrade command). No GitHub issue numbers; this is the implementation branch for Phase 4 in the project status table.
Type of change
feat— new feature (MINOR bump)fix— bug fix (PATCH bump)refactor— no behavior changeperf— performance improvementdocs— documentation onlytest— tests onlychore— maintenance / dependency bumpci— CI/CD onlybuild— build system onlyChecklist
feat(scope): subject)make cilocally and it passes (tidy,fmt,vet,lint,test -race)TestManifestUnmarshalUnioncovers both JSON shapes (falseand codename string) and the existingTestLatestLTS/TestLatestCurrent/TestCacheExpirywere updated to the new struct.docs/, inline godoc) — CHANGELOG[Unreleased]notes both the new command and theLTSCodenamefield change. README's Quick Start already saysnodeup upgrade, so no README edit was needed. Inline godoc onManifestVersion,newUpgradeCmd, and theUnmarshalJSONmethod explains the JSON union..github/workflows/ci.yml)Scope notes / things reviewers may want to look at
runUpgradeusescmd.Printf/cmd.Printlndirectly.CONTRIBUTING.mdsays user-facing output should flow throughinternal/ui, butinternal/ui/is currently an empty package perCLAUDE.md("planned, not yet implemented").check.goandpackages.goalso usecmd.Printfwithout complaint in already-merged PRs (docs: consolidate nodeup.md into README and remove the standalone design doc #9, docs: add status line to managers.md and docs index to README #11, feat(packages): implement snapshot and restore for global npm packages #19), so this PR is consistent with current practice. Happy to introduceinternal/uiin a follow-up PR (Phase 4.5 / 5) and migrate all CLI commands in one pass.contextchecklinter not yet enabled.golangci-lintdoes not currently includecontextcheck, socmd.Context()vscontext.Background()is not enforced by lint.CLAUDE.mddocuments the rule, and the new commits follow it (fix(cli): honor cmd.Context() and surface parse errors in upgrade). Worth enabling the linter in a separateci/lintPR so the rule is enforced project-wide.LTS/TSfields removed. This is a breaking change for any external caller ofnode.ManifestVersion. Within this repo there are no external callers (the field is only consumed byLatestLTS/LatestCurrent, both updated). Worth a0.1.0MINOR bump per the SemVer rule, not a1.0.0MAJOR one, sinceinternal/packages are not Go-module-API stable yet.--no-cleanupand--yesflags are declared but currently no-op. They are wired inrunUpgradewith_ = noCleanup/_ = yesandTODOmarkers pointing at Phase 7 (cleanup) and "non-interactive mode". If you prefer, I can drop them from this PR and add them when the underlying logic lands, rather than ship silent flags. I left them in because theLonghelp text already advertises both behaviors and removing them would be a UX regression for anyone scripting around the (planned) interface.internal/cli/upgrade_test.go. No existinginternal/cli/*.gohas tests in this repo (check.go,list.go,packages.go,config.go,version.goall lack test files), so adding one here would be the first, and would need a non-trivial fake manager. Suggesting a follow-uptest(cli): add upgrade_test.go with fake ManagerPR.Commits on this branch
Per
CONTRIBUTING.md, the PR will squash-merge with the source branch deleted, so the final merge commit subject will befeat(cli): implement end-to-end upgrade command(the first commit's subject).Screenshots / output
nodeup upgrade --dry-runoutput shape:nodeup upgrade(non-dry-run) output shape:Per-version warnings on snapshot/restore failures go to stdout prefixed with
Warning:.