fix: prompt for NGC API key when pulling local NIM images

[zyang-dev]

Summary

Local NIM onboarding fails at the image pull step because docker pull nvcr.io/nim/... requires NGC registry authentication. This adds an NGC API key prompt during onboard that runs docker login nvcr.io --password-stdin before pulling the NIM image. The key is masked during input and handled securely via stdin.

Related Issue

Based on the investigation in PR #236.

Changes

• src/lib/nim.ts: Add isNgcLoggedIn() to check if Docker is already authenticated with nvcr.io, and dockerLoginNgc() to login securely via --password-stdin.
• src/lib/onboard.ts: Prompt for NGC API key before NIM image pull when not already logged in. Masked input, one retry on failure.
• test/onboard-selection.test.ts: Mock isNgcLoggedIn in NIM-local selection test.

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)

AI Disclosure

• AI-assisted — tool: Claude Code

---

Signed-off-by: zyang-dev 267119621+zyang-dev@users.noreply.github.com

Summary by CodeRabbit

• New Features

  • Setup wizard enforces NGC Docker authentication for NIM model setup: interactive mode prompts for an NGC API key (one retry); non-interactive mode prints login instructions and exits.

• Bug Fixes / Reliability

  • Improved detection and login handling for NGC Docker credentials so image pulls proceed only after successful authentication and failures are reported.

• Tests

  • Added unit tests for NGC auth detection and updated onboarding tests to cover authenticated flows.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds two NGC Docker auth helpers to src/lib/nim.ts and integrates an NGC login check into the nim-local onboarding flow. If not logged in, non-interactive onboarding prints docker-login instructions and exits; interactive onboarding prompts for an NGC API key with one retry before pulling NIM images. Tests updated.

Changes

Cohort / File(s)	Summary
NGC Docker helpers src/lib/nim.ts	Add isNgcLoggedIn(): boolean to read/parse ~/.docker/config.json for nvcr.io credentials (credHelpers, auths, credsStore) and dockerLoginNgc(apiKey: string): boolean which runs docker login nvcr.io -u "$oauthtoken" --password-stdin via spawnSync, returning success boolean and logging Docker errors.
Onboarding flow src/lib/onboard.ts	In nim-local path, call nim.isNgcLoggedIn() before pulling images; if not logged in, non-interactive prints docker-login instruction and exits; interactive prompts for NGC API key (secret), normalizes input, attempts nim.dockerLoginNgc() with one retry, and errors/exits on failure.
Tests — onboarding mock test/onboard-selection.test.ts	Augment onboarding test mock to set nimMod.isNgcLoggedIn = () => true so NIM-local tests assume an authenticated state.
Tests — unit src/lib/nim.test.ts	Add describe("isNgcLoggedIn") suite that stubs os.homedir() and fs.readFileSync to validate true/false outcomes across Docker config scenarios: credHelpers, auths entries with auth, https://nvcr.io key, credsStore + marker, missing file, malformed JSON, and empty/absent entries.

Sequence Diagram

sequenceDiagram
    participant User
    participant Wizard as Onboard Wizard
    participant NIM as nim.ts
    participant Config as "~/.docker/config.json"
    participant Docker as "docker CLI"

    User->>Wizard: select NIM-local
    Wizard->>NIM: isNgcLoggedIn()
    NIM->>Config: read / parse config.json
    Config-->>NIM: config contents / error
    NIM-->>Wizard: logged-in? (true/false)

    alt not logged in
        Wizard->>User: prompt for NGC API key (secret)
        User-->>Wizard: provide apiKey
        Wizard->>NIM: dockerLoginNgc(apiKey)
        NIM->>Docker: spawn "docker login nvcr.io -u $oauthtoken --password-stdin"
        Docker-->>NIM: exit status / stderr
        NIM-->>Wizard: success? (true/false)

        alt login failed
            Wizard->>User: re-prompt for API key
            User-->>Wizard: provide apiKey or cancel
            Wizard->>NIM: dockerLoginNgc(apiKey)
            NIM-->>Wizard: final result
        end
    end

    alt logged in or login success
        Wizard->>NIM: pullNimImage(model)
        NIM-->>Wizard: image pulled
    else login failure after retry
        Wizard-->>User: error and exit
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 I sniffed the Docker burrow tight,
I found a key to make things right.
One secret prompt, a retry hop,
Then NIM pulls images—never stop.
— Thimble the Rabbit 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: adding NGC API key prompting when pulling local NIM images.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/local-nim-ngc-login

---

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/nim.ts`:
- Around line 179-189: The current isNgcLoggedIn() does a raw substring search
which yields false positives; change it to parse the Docker config JSON and
check the auths (and optionally credHelpers/credsStore) for an actual entry for
nvcr.io. Specifically, in isNgcLoggedIn() read and JSON.parse the config file,
then confirm that parsed.auths exists and that
Object.keys(parsed.auths).some(key => key.includes("nvcr.io") ||
key.endsWith("nvcr.io")) and that the corresponding parsed.auths[key] has a
non-empty auth/token field; if not found (or JSON parsing fails) return false.

In `@src/lib/onboard.ts`:
- Around line 3958-3978: The NGC login flow currently always calls prompt()
(lines around nim.isNgcLoggedIn and nim.dockerLoginNgc) which will hang in
CI/non-TTY; guard both prompt calls with an isNonInteractive() check and avoid
prompting when non-interactive by failing fast with a clear error (use
processLogger.error or console.error) indicating NGC API key is required or
Docker must be pre-authenticated for non-interactive runs; specifically update
the block that checks nim.isNgcLoggedIn() and the retry branch that calls
prompt() so that if isNonInteractive() is true you skip prompting and
immediately exit (or return a failure) instead of blocking.

🪄 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: Pro Plus

Run ID: 0dad4a20-f07b-4231-80c6-20dcf3b79797

📥 Commits

Reviewing files that changed from the base of the PR and between 800195b and adad6af.

📒 Files selected for processing (3)

• src/lib/nim.ts
• src/lib/onboard.ts
• test/onboard-selection.test.ts

[ericksoa]

Nice work on this PR — the core approach is solid and the security practices (stdin for credentials, masked input, non-interactive guard) are all good. A few things to address before merging:

Should fix

1. Surface Docker error output

dockerLoginNgc() pipes stderr but never shows it to the user. When login fails, the generic "Failed to login" message makes troubleshooting hard (was it a bad key? network error? Docker not running?). Consider logging result.stderr on failure:

if (result.status !== 0 && result.stderr) {
  console.error(`  Docker login error: ${result.stderr.trim()}`);
}

2. Add unit tests for isNgcLoggedIn()

This function has non-trivial JSON parsing logic with multiple code paths (auths, credHelpers, missing file, malformed JSON). Right now the only test coverage is a mock that bypasses it entirely. At minimum, unit tests for the various Docker config shapes would catch regressions.

3. Handle spawnSync error case

If Docker isn't installed or the daemon isn't running, spawnSync returns { status: null, error: ... }. The current code returns false (correct), but the user sees "Failed to login to NGC registry" which is misleading. Checking result.error to surface "Docker not found" or similar would improve the UX.

Nice to have

4. credsStore gap

isNgcLoggedIn() checks credHelpers and auths but not credsStore. Docker Desktop users with a global credential store (e.g., "credsStore": "desktop") may have nvcr.io creds in the OS keychain that don't appear in auths, causing a false negative and unnecessary re-prompt. Worth at least a TODO comment or follow-up issue.

5. Minor nit: $oauthtoken comment

A brief inline comment like // NGC expects literal "$oauthtoken" as username would help future readers unfamiliar with the NGC auth convention.

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/onboard.ts (1)

4023-4049: Extract this NGC login block into a helper to keep setupNim() complexity down.

Line 4035 and Line 4043 duplicate prompt/login handling. Moving this block to a small helper will reduce branching in an already very large function and improve testability.

♻️ Proposed refactor

+async function ensureNgcRegistryLoginForNim() {
+  if (nim.isNgcLoggedIn()) return;
+
+  if (isNonInteractive()) {
+    console.error(
+      "  Docker is not logged in to nvcr.io. In non-interactive mode, run `docker login nvcr.io` first and retry.",
+    );
+    process.exit(1);
+  }
+
+  console.log("");
+  console.log("  NGC API Key required to pull NIM images.");
+  console.log("  Get one from: https://org.ngc.nvidia.com/setup/api-key");
+  console.log("");
+
+  for (let attempt = 0; attempt < 2; attempt += 1) {
+    const ngcKey = normalizeCredentialValue(await prompt("  NGC API Key: ", { secret: true }));
+    if (!ngcKey) {
+      console.error("  NGC API Key is required for Local NIM.");
+      process.exit(1);
+    }
+    if (nim.dockerLoginNgc(ngcKey)) return;
+    console.error("  Failed to login to NGC registry. Check your API key and try again.");
+    console.log("");
+  }
+
+  console.error("  NGC login failed. Cannot pull NIM images.");
+  process.exit(1);
+}
...
-          // Ensure Docker is logged in to NGC registry before pulling NIM images.
-          if (!nim.isNgcLoggedIn()) {
-            ...
-          }
+          await ensureNgcRegistryLoginForNim();

As per coding guidelines, "Limit cyclomatic complexity to 20 in JavaScript/TypeScript files, with target of 15."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard.ts` around lines 4023 - 4049, Extract the NGC login block
inside setupNim() into a small helper (e.g., ensureNgcLoggedIn or
handleNgcLogin) that accepts the nim object and uses
prompt/normalizeCredentialValue/isNonInteractive to perform the logic currently
duplicated around nim.dockerLoginNgc; move all branches there (non-interactive
error + exit, displaying instructions, prompting for NGC API Key, retry once,
and calling process.exit on failure) so setupNim() calls this helper instead of
inline code; reference and reuse nim.dockerLoginNgc, isNonInteractive, prompt,
normalizeCredentialValue and keep the same exit/console behavior to preserve
semantics while reducing cyclomatic complexity and improving testability.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 4023-4049: Extract the NGC login block inside setupNim() into a
small helper (e.g., ensureNgcLoggedIn or handleNgcLogin) that accepts the nim
object and uses prompt/normalizeCredentialValue/isNonInteractive to perform the
logic currently duplicated around nim.dockerLoginNgc; move all branches there
(non-interactive error + exit, displaying instructions, prompting for NGC API
Key, retry once, and calling process.exit on failure) so setupNim() calls this
helper instead of inline code; reference and reuse nim.dockerLoginNgc,
isNonInteractive, prompt, normalizeCredentialValue and keep the same
exit/console behavior to preserve semantics while reducing cyclomatic complexity
and improving testability.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: cb4aabf9-e798-4a47-a9d9-80e8ce31b8d1

📥 Commits

Reviewing files that changed from the base of the PR and between 7b175f6 and ab351d2.

📒 Files selected for processing (1)

• src/lib/onboard.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/nim.ts`:
- Around line 191-194: The isNgcLoggedIn() check currently only returns true for
parsed.credHelpers["nvcr.io"] or explicit auth/token in parsed.auths keys;
update it to also consider a global credential store by returning true if
parsed.credsStore is set (meaning Docker stores credentials externally) and
there is an auths marker for nvcr.io (e.g., auths["nvcr.io"] or
auths["https://nvcr.io"] exists but may be an empty object). In other words,
inside isNgcLoggedIn() use parsed.credsStore || parsed.credHelpers?.["nvcr.io"]
as a positive condition and treat an empty entry in auths as valid when
parsed.credsStore is present, while preserving the existing auth/token checks
for entries.

🪄 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: Pro Plus

Run ID: 02a99a75-6aed-44ce-ab7d-808aec3c62d4

📥 Commits

Reviewing files that changed from the base of the PR and between ab351d2 and 6f584fe.

📒 Files selected for processing (2)

• src/lib/nim.test.ts
• src/lib/nim.ts

✅ Files skipped from review due to trivial changes (1)

• src/lib/nim.test.ts

[coderabbitai]

♻️ Duplicate comments (1)

src/lib/nim.ts (1)

192-195: ⚠️ Potential issue | 🟠 Major

Handle additional Docker auths key variants for nvcr.io.

Line 193 only checks two exact keys. Docker may store registry keys in alternate forms (for example with /v2/), which can make isNgcLoggedIn() return false even when login exists.

🔧 Proposed fix

-    const auths = parsed?.auths || {};
-    const entry = auths["nvcr.io"] || auths["https://nvcr.io"];
-    if (entry?.auth) return true;
+    const auths = parsed?.auths ?? {};
+    const authKey = Object.keys(auths).find((k: string) => /(^|:\/\/)nvcr\.io(\/|$)/i.test(k));
+    const entry = authKey ? auths[authKey] : undefined;
+    if (entry?.auth || entry?.identitytoken || entry?.token) return true;
     if (entry && parsed?.credsStore) return true;

#!/bin/bash
set -euo pipefail

# Inspect current matching logic in src/lib/nim.ts
rg -n -C3 'isNgcLoggedIn|auths\["nvcr\.io"\]|https://nvcr\.io|credsStore|identitytoken|token' src/lib/nim.ts

# Verify whether tests cover non-exact nvcr.io auth key variants (e.g. /v2/)
rg -n -C2 'nvcr\.io|https://nvcr\.io|/v2/' --iglob '*nim*.test.ts'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/nim.ts` around lines 192 - 195, The login check in isNgcLoggedIn only
tests auths["nvcr.io"] and auths["https://nvcr.io"], which misses variants like
"https://nvcr.io/v2/" or other prefixed/suffixed keys; update isNgcLoggedIn to
iterate Object.keys(parsed?.auths || {}) and match keys against a regex that
normalizes/removes scheme and path (e.g. /^(?:https?:\/\/)?nvcr\.io(?:\/.*)?$/i)
to find the nvcr entry, then treat an entry as logged-in if it has auth,
identitytoken, token, or if parsed?.credsStore is set; ensure you still use
parsed?.auths and parsed?.credsStore and update references to auths/entry
accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/lib/nim.ts`:
- Around line 192-195: The login check in isNgcLoggedIn only tests
auths["nvcr.io"] and auths["https://nvcr.io"], which misses variants like
"https://nvcr.io/v2/" or other prefixed/suffixed keys; update isNgcLoggedIn to
iterate Object.keys(parsed?.auths || {}) and match keys against a regex that
normalizes/removes scheme and path (e.g. /^(?:https?:\/\/)?nvcr\.io(?:\/.*)?$/i)
to find the nvcr entry, then treat an entry as logged-in if it has auth,
identitytoken, token, or if parsed?.credsStore is set; ensure you still use
parsed?.auths and parsed?.credsStore and update references to auths/entry
accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f31d5ba7-3994-4a6b-b9fd-a481b5624263

📥 Commits

Reviewing files that changed from the base of the PR and between 6f584fe and 5c43718.

📒 Files selected for processing (2)

• src/lib/nim.test.ts
• src/lib/nim.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/lib/nim.test.ts

[ericksoa]

LGTM. Clean implementation of NGC registry auth for local NIM onboarding.

• API key piped via --password-stdin (never in argv) — good security practice
• Docker Desktop credential storage correctly handled (empty marker + credsStore)
• Both nvcr.io and https://nvcr.io variants checked in isNgcLoggedIn()
• Non-interactive mode exits with clear instructions instead of hanging
• 10 test cases for auth detection cover the key scenarios
• One retry on failure is pragmatic UX