Skip to content

refactor(installer): extract pure domain helpers#3107

Merged
cv merged 108 commits into
mainfrom
refactor/installer-domain-helpers
May 6, 2026
Merged

refactor(installer): extract pure domain helpers#3107
cv merged 108 commits into
mainfrom
refactor/installer-domain-helpers

Conversation

@cv
Copy link
Copy Markdown
Collaborator

@cv cv commented May 6, 2026

Summary

Extracts pure installer decision helpers into typed domain modules as the first step toward moving installer behavior out of shell. This adds TypeScript coverage for release ref resolution, runtime version checks, provider normalization, and npm prefix/link target decisions without changing installer shell behavior yet.

Changes

  • Add src/lib/domain/installer/version.ts for semver/runtime support checks.
  • Add src/lib/domain/installer/provider.ts for installer provider aliases and help values.
  • Add src/lib/domain/installer/ref.ts for install ref and installer version source precedence.
  • Add src/lib/domain/installer/npm.ts for npm global bin/path and link-target writability decisions.
  • Add unit tests covering parity with the current shell helper semantics.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • make docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

  • Tests
    • Added comprehensive test coverage for installer infrastructure including provider configuration, version compatibility checks, reference resolution, and npm path handling.

cv added 30 commits May 2, 2026 13:36
@cv cv self-assigned this May 6, 2026
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 6, 2026

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 6, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 4c7ce911-94fd-4c61-b89b-e348062e64f8

📥 Commits

Reviewing files that changed from the base of the PR and between 28daa3e and dbf7088.

📒 Files selected for processing (2)
  • src/lib/domain/installer/provider.test.ts
  • src/lib/domain/installer/provider.ts

📝 Walkthrough

Walkthrough

This PR introduces a new installer domain module comprising four interconnected TypeScript utilities for npm installer management. The changes add type definitions and functions for version checking, installer provider resolution, install reference handling, and npm-specific path utilities, along with comprehensive test coverage for each module.

Changes

Installer Domain Foundation

Layer / File(s) Summary
Version Utilities & Runtime Checks
src/lib/domain/installer/version.ts
Establishes MIN_NODE_VERSION and MIN_NPM_MAJOR constants; defines RuntimeCheckResult type; provides normalizeVersion, versionMajor, and versionGte utilities for semantic version handling and comparison; implements checkInstallerRuntime to validate node and npm versions against specified minimums.
Provider Type System
src/lib/domain/installer/provider.ts
Introduces InstallerProvider type and INSTALLER_PROVIDER_VALUES constant with read-only tuple. Provides INSTALLER_PROVIDER_ALIASES mapping for case-insensitive alias resolution; exports normalizeInstallerProvider for validation and installerProviderHelpValues and installerProviderUsageLines for documentation generation.
Reference & Version Resolution
src/lib/domain/installer/ref.ts
Defines InstallerRefEnv interface for environment variable injection. Provides resolveInstallRef to prioritize explicit ref, tag, or "latest" default; implements installerVersionFromRef and resolveInstallerVersion to derive semantic versions from refs, git-describe, stamped, and packageJson sources with fallback chain.
npm Path & Writability
src/lib/domain/installer/npm.ts
Defines NpmLinkTargetPaths and NpmLinkTargetState interfaces; exports npmGlobalBin to compute npm's global binary directory; implements npmLinkTargetPaths to derive standard npm link structure (bin, lib, node_modules); provides npmLinkTargetsWritable with explicit failure reasons (empty-prefix, bin-unwritable, lib-unwritable); includes pathWithPrependedEntries for deduplicative PATH concatenation.
Tests & Validation
src/lib/domain/installer/version.test.ts, src/lib/domain/installer/provider.test.ts, src/lib/domain/installer/ref.test.ts, src/lib/domain/installer/npm.test.ts
Version module tests cover shell-like version semantics, major version extraction, and runtime classification across node/npm combinations. Provider tests validate normalization of aliases and help text formatting. Ref tests verify priority ordering of env refs and version resolution fallback chain. npm tests confirm global bin resolution, path structure, writability scenarios, and PATH deduplication.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A installer domain takes shape today,
With versions checked and paths on display,
npm targets found, their writability sure,
Provider aliases, references pure—
A foundation for installation to endure! 🔧

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: extracting pure domain helpers for the installer module into new TypeScript modules.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/installer-domain-helpers

Comment @coderabbitai help to get the list of available commands and usage tips.

@cv cv added NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). refactor This is a refactor of the code and/or architecture. v0.0.36 Release target labels May 6, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/domain/installer/provider.ts`:
- Around line 36-45: The help/usage text in installerProviderHelpValues() and
installerProviderUsageLines() is missing the accepted alias
"anthropiccompatible" and can drift from normalizeInstallerProvider; update the
hardcoded strings to include "anthropiccompatible" (matching the value accepted
by normalizeInstallerProvider) and, to prevent future drift, refactor to
export/use a single canonical list of provider/alias names (e.g., a const
PROVIDER_NAMES or PROVIDER_ALIASES) referenced by installerProviderHelpValues(),
installerProviderUsageLines(), and normalizeInstallerProvider so all three read
from the same source of truth.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a07181f3-ce79-430b-b4e8-bb1c1b008c05

📥 Commits

Reviewing files that changed from the base of the PR and between 9af5206 and 28daa3e.

📒 Files selected for processing (8)
  • src/lib/domain/installer/npm.test.ts
  • src/lib/domain/installer/npm.ts
  • src/lib/domain/installer/provider.test.ts
  • src/lib/domain/installer/provider.ts
  • src/lib/domain/installer/ref.test.ts
  • src/lib/domain/installer/ref.ts
  • src/lib/domain/installer/version.test.ts
  • src/lib/domain/installer/version.ts

Comment thread src/lib/domain/installer/provider.ts
@cv
Copy link
Copy Markdown
Collaborator Author

cv commented May 6, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 6, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cv
Copy link
Copy Markdown
Collaborator Author

cv commented May 6, 2026

Automated PR review summary

Reviewed PR #3107: refactor(installer): extract pure domain helpers

Recommendation

  • Recommendation: PASS
  • Highest observed severity: low
  • Block merge: no
  • Why: If this refactor had diverged from shell semantics, the highest downside would be incorrect installer ref selection, misleading provider handling, or broken npm path/runtime gating. The adversarial parity probes I ran all passed, reducing confidence that the extraction introduced behavioral drift.
  • Reviewer summary: Reviewed PR refactor(installer): extract pure domain helpers #3107 with targeted adversarial probes focused on the specific refactor claims. The extracted installer domain helpers preserved current installer shell behavior on the highest-risk edge cases I tested, so I found no regression evidence in this environment.

Installation and setup findings

  • Things went well: Installed the locally checked out NemoClaw from /workspace/nemoclaw via the repo root installer with source-checkout environment overrides, then verified the NemoClaw-managed sandbox was usable over SSH and could reach the configured inference provider from inside the sandbox.

What was validated

  • The PR revision was checked out in an isolated review environment.
  • The local checkout was installed using the repository installer flow as closely as the environment allowed.
  • Adversarial, PR-specific probes were then run against the installed environment and relevant repository context.
  • Diff summary:
 .../skills/nemoclaw-contributor-create-pr/SKILL.md |   20 +-
 .../nemoclaw-maintainer-pr-comparator/SKILL.md     |  121 --
 .../checks/tier-0-gates.md                         |   61 -
 .../checks/tier-1-correctness.md                   |   86 --
 .../checks/tier-2-quality.md                       |   64 -
 .../repo-policy.md                                 |   86 --
 .../scripts/check-coderabbit-threads.sh            |  105 --
 .../scripts/collect-gates.sh                       |   84 --
 .../scripts/find-candidates.sh                     |   86 --
 .../scripts/parse-supersession.sh                  |   68 -
 .../scripts/render-verdict.py                      |  232 ----
 .../templates/verdict.md                           |   88 --
 .../tiebreakers.md                                 |   61 -
 .../validation/backtest.md                         |   69 -
 .agents/skills/nemoclaw-skills-guide/SKILL.md      |    2 +-
 .../references/agent-skills.md                     |    4 +-
 .../nemoclaw-user-configure-inference/SKILL.md     |  329 ++---
 .../references/inference-options.md                |    4 +-
 .../references/set-up-sub-agent.md                 |  120 --
 .../references/switch
...[truncated]

Failing tests and unresolved impact

  • No failing adversarial tests were captured.

Passing tests and why they mattered

Passing test 1: Provider normalization/help parity

  • What was tested: The extracted provider helpers preserve installer shell aliases, case/whitespace normalization, and published help/usage values.
  • Why it mattered: If false, users could see misleading provider help text or have previously accepted provider values start failing after future wiring to these helpers.
  • Observed result: Helper normalized cloud->build, nim->nim-local, trimmed AnthropiCompatible, rejected unsupported, and produced the expected provider help/usage strings; parity summary reported match=true.
  • Command: bash /tmp/pr3107-test1.sh
  • Recommended follow-up coverage: Keep the unit tests and consider one integration regression test asserting installer help output stays sourced from the same canonical provider list once these helpers are wired into live output.

Passing test 2: Install ref/version precedence parity

  • What was tested: The extracted ref helpers preserve shell precedence between NEMOCLAW_INSTALL_REF, NEMOCLAW_INSTALL_TAG, and fallback version sources.
  • Why it mattered: If false, installer bootstrap could fetch the wrong ref or report the wrong version, which is a meaningful release-management regression.
  • Observed result: Observed REF>tag, whitespace-only REF fallback to TAG, default latest, and version derivation from ref/git-describe/stamped/package fallback exactly matched expected shell-style precedence.
  • Command: bash /tmp/pr3107-test2.sh
  • Recommended follow-up coverage: Keep as unit coverage and add a regression case for whitespace-only env vars because env precedence logic often regresses during shell-to-domain refactors.

Passing test 3: npm link-target writability parity

  • What was tested: The extracted npm helpers preserve shell decisions for bin/lib/node_modules writability across adversarial path-state combinations.
  • Why it mattered: If false, installer could choose the wrong link vs shim path or misreport permission failures, breaking install reliability on real hosts.
  • Observed result: Adversarial cases for missing bin target, unwritable lib dir, unwritable node_modules, and empty prefix matched shell accept/reject outcomes; helper reasons were bin-unwritable, lib-unwritable, and empty-prefix as expected.
  • Command: bash /tmp/pr3107-test3.sh
  • Recommended follow-up coverage: Keep the unit tests and add an integration/regression test around live installer link-vs-shim path selection when these helpers become the production decision path.

Passing test 4: Runtime version gate parity

  • What was tested: The extracted version helpers preserve shell semver comparison and installer runtime gate behavior, including short versions and prerelease rejection.
  • Why it mattered: If false, supported runtimes could be rejected or unsupported runtimes accepted, causing install failures or undefined behavior.
  • Observed result: Semver parity held for equal, higher/lower minor/patch, short 22.16 vs 22.16.0, and prerelease rejection; runtime classifications matched supported, unsupported, invalid-node-version, and invalid-npm-version expectations.
  • Command: bash /tmp/pr3107-test4.sh
  • Recommended follow-up coverage: Keep as unit tests; extra integration coverage is only needed once these helpers directly replace the live shell runtime checks.

Bottom line

  • Based on the install evidence and adversarial probes, this PR looks reasonable to approve.

Copy link
Copy Markdown
Contributor

@cjagwani cjagwani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pure additions, 1:1 faithful to install.sh equivalents (npm_link_targets_writable, version_gte, getNonInteractiveProvider, resolve_release_tag). 40 assertions across 10 test cases cover empty/whitespace/prerelease/multi-failure-mode edges. type sigs are discriminated unions, no narrowing regressions.

@cv cv marked this pull request as ready for review May 6, 2026 21:02
@cv cv changed the base branch from test/canonical-sandbox-commands to main May 6, 2026 21:02
@cv cv enabled auto-merge (squash) May 6, 2026 21:02
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv requested a review from prekshivyas May 6, 2026 21:03
Copy link
Copy Markdown
Contributor

@prekshivyas prekshivyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pure additive helper extraction, same staging as #3080#3083. install.sh is untouched; this lands the typed planning layer first. Four typed modules + four matching test files under src/lib/domain/installer/:

  • version.tsversionGte, versionMajor, checkInstallerRuntime with constants MIN_NODE_VERSION = \"22.16.0\", MIN_NPM_MAJOR = 10. Tagged result with reason codes (invalid-node-version / invalid-npm-version / unsupported).
  • provider.ts — 9 canonical provider values + 3 aliases (anthropiccompatible, cloud, nim); case/trim/alias normalization in normalizeInstallerProvider.
  • ref.tsresolveInstallRef (env priority NEMOCLAW_INSTALL_REF > NEMOCLAW_INSTALL_TAG > \"latest\"), resolveInstallerVersion with multi-source fallback (ref-version > git-describe > stamped > package.json > default).
  • npm.tsnpmLinkTargetsWritable with tagged result (bin-unwritable / lib-unwritable / empty-prefix) and chained existence/writability checks; pathWithPrependedEntries (no duplicates).

Test coverage is the right shape for a parity-with-shell extraction: edge cases (0.18.1 ≥ 0.18.0, 22.16 vs 22.16.0, RC tags rejected, all 4 writability outcomes including chained fallback to prefix-writability), round-trip invariants (every INSTALLER_PROVIDER_VALUES entry and every alias normalizes correctly), and explicit help-text string matches that act as a regression net against install.sh divergence.

Observations (non-blocking):

  1. MIN_NODE_VERSION / MIN_NPM_MAJOR duplicate the values install.sh uses today. A comment pointing at the canonical shell source would help future maintainers know where to update both, but this matches the duplication pattern accepted in the #3080 uninstall extraction.
  2. installerVersionFromRef edge case: ref = \"v\" returns fallbackVersion rather than null. Minor, not exercised by tests but consistent with the "strip v, fall back if empty" intent.

CI: pr.yaml lightweight checks pass (commit-lint, dco, layer-boundary, check-hash, changes). checks / macos-e2e / 2 self-hosted runs still in progress at approval time.

@cv cv merged commit 538630e into main May 6, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). refactor This is a refactor of the code and/or architecture. v0.0.36 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants