fix(detector): align asdf env var on ASDF_DATA_DIR across detector, classifier, docs#84
Merged
Merged
Conversation
…lassifier, docs The asdf branch of `managerManagedRoots` (system_node.go) was reading `$ASDF_DIR` to discover the manager's data dir, while `asdfDataDir()` (the function the detector's actual `Install`/`ListInstalled` paths shell out against) reads `$ASDF_DATA_DIR` — and `docs/managers.md` documented `$ASDF_DIR` in the supported-manager table. The three were inconsistent. A user with `ASDF_DIR=/path/to/asdf-source` (the source checkout from a git-clone install) would have nodeup classify the `node` binary under that source dir as `manager`-managed even though the manager's actual install location was elsewhere (typically `~/.asdf`). Conversely, a user with the canonical `ASDF_DATA_DIR` override would have it silently ignored by the path classifier, leaving their manager install to be reported as `unknown` in `nodeup check` and triggering the upgrade-time warning. Unify all three on `$ASDF_DATA_DIR`, matching the variable `asdfDataDir()` already used and the official asdf docs (the source-checkout variable is `$ASDF_DIR`; the data dir override is `$ASDF_DATA_DIR`). Update the `system_node_test.go` "asdf env wins" case to drive the new variable, and `detector.go`'s package doc comment to match. Closes #50. Co-Authored-By: puku-ai-2.8 <noreply@puku.sh>
|
✔️ 9707118 - Conventional commits check succeeded. |
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
The asdf branch of
internal/detector/system_node.go'smanagerManagedRootswas reading$ASDF_DIRto discover the manager's data dir, whileinternal/detector/asdf.go::asdfDataDir()(the function the detector's actualInstall/ListInstalledpaths shell out against) reads$ASDF_DATA_DIR— anddocs/managers.mddocumented$ASDF_DIRin the supported-manager table. The three were inconsistent.A user with
ASDF_DIR=/path/to/asdf-source(the source checkout from a git-clone install) would have nodeup classify thenodebinary under that source dir asmanager-managed even though the manager's actual install location was elsewhere (typically~/.asdf). Conversely, a user with the canonicalASDF_DATA_DIRoverride would have it silently ignored by the path classifier, leaving their manager install to be reported asunknowninnodeup checkand triggering the upgrade-time warning.Unify all three on
$ASDF_DATA_DIR, matching the variableasdfDataDir()already used and the official asdf docs (the source-checkout variable is$ASDF_DIR; the data-dir override is$ASDF_DATA_DIR). Updatesystem_node_test.go's "asdf env wins" case to drive the new variable, anddetector.go's package doc comment to match.Closes #50.
Linked issues
Type of change
fix— bug fix (PATCH bump)Checklist
fix(scope): subject)make cilocally and it passes (fmt, vet, test pass locally;make lintfails on the v2→v1 schema mismatch tracked in build(lint): .golangci.yml v1 schema incompatible with golangci-lint v2 (brew installs v2, make lint fails to even parse config) #62 — pre-existing onmain, independent of this fix; CI runs golangci-lint v1.64.8 per.github/workflows/ci.ymland parses the config fine)system_node_test.go's "asdf env wins" case to useASDF_DATA_DIRinstead ofASDF_DIR; existingasdf_test.go::TestASDF_Detect_HonorsASDF_DATA_DIRand theASDF_DATA_DIRdata-dir resolution tests already cover the rest of the contract)docs/, inline godoc)Scope notes
internal/detector/detector.gohad a package-doc comment listing the supported env vars includingASDF_DIR. Updated toASDF_DATA_DIR.$ASDF_DIRas an additional fallback (some installations use it as an alias for the data dir). The official asdf docs distinguish the two cleanly:$ASDF_DIRis the source checkout,$ASDF_DATA_DIRis the data dir. If a user is using$ASDF_DIRto mean "data dir" they should switch to$ASDF_DATA_DIR— the same variable the rest of their tooling already uses.Out of scope (separate issues)
--cleanup/--yesprecedence conflict — bug(cli): --yes silently overrides --no-cleanup, auto-deletes with zero confirmation #57.packages restore/diffvia unsanitized manager name — fix(packages): path traversal via unsanitized manager name in packages restore/diff #51.Current()failure leaves active Node unprotected from cleanup — bug(cli): Current() failure leaves the active Node version unprotected from cleanup deletion #58.Current()relies on unverifiedn currentsubcommand — fix(detector): n manager's Current() relies on unverified 'n current' subcommand #59.Screenshots / output
User-visible behavior change:
ASDF_DIR=/path/to/sourceset in their env (typical for git-clone installs) seesnodeup checkprint a warning that theirnodebinary isn't manager-managed even though asdf fully owns it. Conversely, a user withASDF_DATA_DIR=/customsees the classifier claim theirnodeis "manager" only when it happens to land under~/.asdf(the fallback path).ASDF_DATA_DIRis the single source of truth: setting it points the classifier at the actual data dir, regardless of what$ASDF_DIRhappens to contain.Verification path:
go test -race ./internal/detector/... -run TestSystemNode_ManagerManagedRoots -v(the test table now exercisesASDF_DATA_DIRdirectly).