Fix all unresolved CodeRabbit comments on this PR:

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

src/lib/onboard.ts (3)

6812-6827: ⚠️ Potential issue | 🟡 Minor

Abort message can overstate guarantees after prior flush.

If an earlier loop iteration already flushed deferred credentials, this path still prints “No credentials were persisted to disk.” That statement can be false.

Proposed fix

-    let forceProviderSelection = false;
+    let forceProviderSelection = false;
+    let deferredCredentialsFlushed = false;
@@
         if (answer === "n" || answer === "no") {
@@
-          console.log("  No credentials were persisted to disk.");
+          if (deferredCredentialsFlushed) {
+            console.log("  Remaining deferred credentials were discarded.");
+            console.log("  Note: previously confirmed credentials may already be on disk.");
+          } else {
+            console.log("  No credentials were persisted to disk.");
+          }
           process.exit(0);
         }
       }
@@
       if (deferInferenceCredentials) {
         flushDeferredCredentials();
+        deferredCredentialsFlushed = true;
       }

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 6812 - 6827, The abort path prints "No
credentials were persisted to disk." even if a prior iteration already called
flushDeferredCredentials(); introduce or check a flag that tracks whether
deferred credentials have been flushed (e.g., deferredCredentialsFlushed) and
only call the “No credentials were persisted to disk.” message when no flush has
occurred, or change the messaging to reflect that credentials may have been
persisted earlier; update the logic around discardDeferredCredentials(),
flushDeferredCredentials(), and deferInferenceCredentials to consult this flag
so the printed statement accurately reflects whether any flush happened.

---

6726-6734: ⚠️ Potential issue | 🟠 Major

Deferral is armed, but NVIDIA key entry still bypasses it.

Deferral is enabled here, but Line 4376 still uses ensureApiKey() in the NVIDIA path, which is outside this deferred write path. That leaves a pre-gate persistence path for NVIDIA credentials.

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 6726 - 6734, The NVIDIA credential branch
still calls ensureApiKey(), causing immediate persistence outside the deferred
flow started by deferInferenceCredentials/beginDeferredCredentialPersistence;
update the NVIDIA path to use the deferred credential flow instead of
ensureApiKey(): either call the credential collection function that queues
writes into the deferred store (or create/route through a new
ensureDeferredApiKey helper) so writes go through the same deferred persistence
mechanism controlled by deferInferenceCredentials and finalized at the review
gate, and remove any direct calls that persist to ~/.nemoclaw/credentials.json
from the NVIDIA branch.

---

757-761: ⚠️ Potential issue | 🟠 Major

Deferred staging can persist abandoned provider credentials.

Line 760 adds every entered env key to pendingDeferredCredentials, but abandoned selections are never evicted before flushDeferredCredentials(). This can write credentials that are no longer part of the final confirmed selection.

Proposed fix

     if (deferCredentialPersistence) {
+      // Keep only the current in-progress inference credential staged.
+      // If the user backs out and chooses a different provider, do not
+      // retain abandoned staged credentials for a later flush.
+      for (const stagedEnvName of [...pendingDeferredCredentials]) {
+        if (stagedEnvName !== envName) {
+          pendingDeferredCredentials.delete(stagedEnvName);
+          delete process.env[stagedEnvName];
+        }
+      }
       // Hold in memory + process.env; flushDeferredCredentials() will write
       // to ~/.nemoclaw/credentials.json after the review gate confirms.
       pendingDeferredCredentials.add(envName);
       process.env[envName] = key;

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 757 - 761, pendingDeferredCredentials
currently adds every envName when deferCredentialPersistence is true, which
leaves abandoned selections in the set and causes flushDeferredCredentials() to
persist stale credentials; fix by only adding envName to
pendingDeferredCredentials when the provider/credential is actually confirmed
(i.e., move the add(envName) into the confirmation/acceptance path) or ensure
every abort/cancel/change flow removes it (call
pendingDeferredCredentials.delete(envName) and delete process.env[envName]) so
only confirmed envNames remain for flushDeferredCredentials; update the code
paths around deferCredentialPersistence, pendingDeferredCredentials, envName and
flushDeferredCredentials to implement one of these strategies.

src/lib/onboard.ts (1)

734-736: Scrub staged env vars when discarding deferred credentials.

discardDeferredCredentials() disarms and clears the set, but leaves staged secret values in process.env for the remainder of the process. Clearing those env vars is safer and keeps behavior robust if this function is reused without immediate process exit.

Proposed fix

 function discardDeferredCredentials() {
+  for (const envName of pendingDeferredCredentials) {
+    delete process.env[envName];
+  }
   pendingDeferredCredentials.clear();
   deferCredentialPersistence = false;
 }

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 734 - 736, discardDeferredCredentials
currently clears pendingDeferredCredentials and flips deferCredentialPersistence
but leaves any staged secret values in process.env; update
discardDeferredCredentials to iterate the pendingDeferredCredentials set (e.g.,
for each key in pendingDeferredCredentials) and delete those entries from
process.env before clearing the set and setting deferCredentialPersistence =
false so staged env vars are scrubbed from the process when credentials are
discarded.

src/lib/onboard.ts (2)

6876-6883: ⚠️ Potential issue | 🟠 Major

Abort can over-promise after a previously confirmed retry.

If an earlier loop iteration already hit flushDeferredCredentials(), aborting here still says “No credentials were persisted to disk.” That becomes false as soon as any previous retry was confirmed and flushed.

Also applies to: 6886-6891

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 6876 - 6883, The abort messaging incorrectly
always claims “No credentials were persisted to disk.” even when a prior retry
already called flushDeferredCredentials(); update the abort branches (the block
that currently calls discardDeferredCredentials() and the similar block at
6886-6891) to check whether deferred credentials were already flushed and
conditionally log the correct message: if flushDeferredCredentials() (or a
boolean flag set when flushDeferredCredentials() runs) indicates credentials
were written, log that credentials were persisted and provide guidance;
otherwise call discardDeferredCredentials() and log that no credentials were
persisted. Ensure you reference and use the existing functions
flushDeferredCredentials() and discardDeferredCredentials() (or set/read a
flushed flag where flushDeferredCredentials() runs) so the message accurately
reflects state before calling process.exit(0).

---

715-727: ⚠️ Potential issue | 🟠 Major

Backing out to another provider can still flush the abandoned key.

The env name is staged as soon as the key is entered, and flushDeferredCredentials() later persists every staged name. I still don't see any "back" / "selection" path removing the abandoned env name or process.env value before the user picks a different provider, so confirming the second choice can write both credentials.

Also applies to: 757-761

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 715 - 727, The current
flushDeferredCredentials() unconditionally persists any env names in
pendingDeferredCredentials, allowing an abandoned/previous provider's key to be
saved; fix by tracking the staged credential value when staging (store a map
stagedCredentialValues: Map<string,string> alongside
pendingDeferredCredentials), then in flushDeferredCredentials() only call
saveCredential(envName, value) if process.env[envName] ===
stagedCredentialValues.get(envName) and the value is non-empty; also ensure any
“back”/“selection” or cancellation paths remove the envName from
pendingDeferredCredentials and stagedCredentialValues (and clear
process.env[envName] if appropriate) so abandoned entries cannot be
flushed—refer to flushDeferredCredentials, pendingDeferredCredentials,
saveCredential, endDeferredCredentialPersistence and the code that stages env
names when a key is entered to implement this.

src/lib/onboard.ts (1)

6795-6798: Pair the deferral lifecycle with a finally.

If anything throws between beginDeferredCredentialPersistence() and endDeferredCredentialPersistence(), the module-scoped flag/set stay armed for the rest of the process. Wrapping this retry loop in a try/finally would avoid leaking deferred state into later prompts or tests.

Also applies to: 6916-6918

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 6795 - 6798, The code arms deferred
credential persistence via deferInferenceCredentials and calls
beginDeferredCredentialPersistence() but does not guarantee
endDeferredCredentialPersistence() runs on error; wrap the block that runs while
deferInferenceCredentials is true (the retry loop and any logic between
beginDeferredCredentialPersistence() and endDeferredCredentialPersistence()) in
a try/finally so that endDeferredCredentialPersistence() is always called in the
finally clause (also apply the same change to the other occurrence around lines
6916-6918); reference deferInferenceCredentials,
beginDeferredCredentialPersistence, and endDeferredCredentialPersistence to
locate and protect the lifecycle.

Fix all unresolved CodeRabbit comments on this PR:

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

src/lib/onboard.ts (1)

791-795: ⚠️ Potential issue | 🟡 Minor

Avoid double-printing the save banner in the immediate path.

If saveCredential() still owns the interactive credentials.json success output, these lines will print the same status twice. Please verify the helper and keep the message in one place only.

#!/bin/bash
set -euo pipefail

echo "== saveCredential definition / output =="
rg -n -C4 '\bsaveCredential\s*\(|credentials\.json' src/lib/credentials.ts

echo
echo "== immediate-save branch in onboard.ts =="
sed -n '788,796p' src/lib/onboard.ts

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 791 - 795, The code prints the
credentials.json success banner twice: once inside saveCredential(...) and again
immediately after in the onboard immediate-save branch (the console.log block
that references ~/.nemoclaw/credentials.json); inspect the saveCredential
implementation in credentials.ts to see if it already logs the success message,
then remove the duplicate output by either deleting the console.log(...) lines
here (the immediate-save block that uses envName and key) or suppressing logging
inside saveCredential so only one place logs the success—ensure only one of the
two (saveCredential or the onboard immediate-save branch) emits the
credentials.json save banner.

src/lib/onboard.ts (1)

6861-7007: Extract the deferred review-gate orchestration out of onboard().

This patch adds more stateful branching to an already complexity-suppressed retry loop. Pulling the defer/flush/discard/summary flow into a dedicated helper would make this path much easier to reason about and test.

As per coding guidelines, TypeScript files must maintain cyclomatic complexity limit of 20, ratcheting down to 15. Enforced by ESLint.

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 6861 - 7007, The review gate and
deferred-credential orchestration (calculation of deferInferenceCredentials,
beginDeferredCredentialPersistence(), the summary/confirmation prompt block that
calls promptValidatedSandboxName(), formatOnboardConfigSummary(),
promptOrDefault(), discardDeferredCredentials(),
haveDeferredCredentialsBeenFlushed(), flushDeferredCredentials(), and
endDeferredCredentialPersistence()) should be extracted from onboard() into a
single helper (e.g., handleDeferredCredentialReviewGate or
confirmInferenceCredentials) that accepts the mutable pieces it needs (provider,
model, credentialEnv, webSearchConfig, selectedMessagingChannels, sandboxName,
dangerouslySkipPermissions, isNonInteractive, etc.) and returns the updated
sandboxName plus a flag indicating whether to proceed or abort; replace the
inline block with a call to this helper, ensure
beginDeferredCredentialPersistence()/endDeferredCredentialPersistence() are
paired inside the helper, preserve the priorFlush logic and process.exit
behavior by returning an abort signal instead of exiting deep in onboarding, and
add unit tests for the new helper to reduce onboard() cyclomatic complexity to
satisfy ESLint limits.

src/lib/onboard.ts (1)

720-745: ⚠️ Potential issue | 🔴 Critical

Staged keys still need to be visible to every later lookup path.

These helpers now rely on process.env/pendingDeferredCredentials, but this file still re-reads credentials via getCredential(...) during selection validation and via hydrateCredentialEnv() at the top of setupInference(). If src/lib/credentials.ts#getCredential is still reading persisted storage, a freshly entered key stays invisible before confirm, and a post-confirm "retry credential" inside setupInference() will fall back to the old saved key on the next loop. Either make the shared lookup prefer staged env values or stop deferring once the first review gate has been accepted.

#!/bin/bash
set -euo pipefail

echo "=== credential lookup implementation ==="
rg -n -C4 '\bfunction getCredential\b|\bfunction saveCredential\b|process\.env|credentials\.json' src/lib/credentials.ts

echo
echo "=== onboard call sites that re-read credentials after staging ==="
rg -n -C2 '\bgetCredential\(|\bhydrateCredentialEnv\(' src/lib/onboard.ts

Expected result: getCredential() (or a shared helper used by these call sites) should prefer staged process.env values before persisted storage while deferral is active.

Also applies to: 795-803

🤖 Prompt for AI Agents

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

In `@src/lib/onboard.ts` around lines 720 - 745, The staged credential values
aren't being honored by later lookups in setupInference and selection
validation; update the shared lookup logic so getCredential (or the helper used
by hydrateCredentialEnv and setupInference) prefers staged values when
deferCredentialPersistence is active by first checking process.env[envName] and
pendingDeferredCredentials for a non-empty stagedValue before falling back to
persisted storage, or alternatively clear deferCredentialPersistence (call
endDeferredCredentialPersistence()) immediately after the first review/confirm
gate is accepted so subsequent getCredential/hydrateCredentialEnv calls see the
freshly staged value; touch the functions getCredential, hydrateCredentialEnv,
setupInference, flushDeferredCredentials, and endDeferredCredentialPersistence
to implement this consistent precedence.