feat(cli): add 'nemoclaw credentials' command for resetting stored keys#1597
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a top-level Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "nemoclaw CLI"
participant Cred as "credentials module"
participant FS as "filesystem"
User->>CLI: run "nemoclaw credentials reset <KEY>"
CLI->>Cred: call listCredentialKeys()
Cred->>FS: read `credentials.json`
FS-->>Cred: credentials data
Cred-->>CLI: key list
CLI->>User: prompt confirmation via askPrompt
User-->>CLI: confirm / cancel
alt confirmed
CLI->>Cred: call deleteCredential(<KEY>)
Cred->>FS: read `credentials.json`
Cred->>FS: write updated `credentials.json`
FS-->>Cred: write result
Cred-->>CLI: deletion success
CLI->>User: print success and exit 0
else cancelled or failure
CLI->>User: print error/message and exit 1
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/credentials.test.js (1)
67-82: Test intent and assertions are slightly misaligned for file mode.Line 67 says “leaves the file mode intact,” but the test never asserts file permissions. Add a
fs.statSync(...).mode & 0o777check before/after deletion to lock this guarantee in.Proposed test assertion addition
it("deleteCredential removes a stored key and leaves the file mode intact", async () => { const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); const credentials = await importCredentialsModule(home); credentials.saveCredential("NVIDIA_API_KEY", "nvapi-bad-key"); credentials.saveCredential("OTHER_KEY", "other-value"); + const credsFile = path.join(home, ".nemoclaw", "credentials.json"); + expect(fs.statSync(credsFile).mode & 0o777).toBe(0o600); expect(credentials.listCredentialKeys()).toEqual(["NVIDIA_API_KEY", "OTHER_KEY"]); expect(credentials.deleteCredential("NVIDIA_API_KEY")).toBe(true); + expect(fs.statSync(credsFile).mode & 0o777).toBe(0o600); expect(credentials.getCredential("NVIDIA_API_KEY")).toBe(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credentials.test.js` around lines 67 - 82, The test description claims "leaves the file mode intact" but never checks file permissions; add assertions using fs.statSync to capture the credential file's permission mode (e.g., fs.statSync(<credential file path>).mode & 0o777) before calling credentials.deleteCredential("NVIDIA_API_KEY") and again after deletion and assert they are equal; locate the credential file used by importCredentialsModule/home in this test and place the two mode checks around the deleteCredential call so the test verifies the file mode is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 927-955: The validation for the "credentials reset" command
incorrectly uses getCredential(key) which can return values from process.env;
change the check to only consult the persisted keys via listCredentialKeys()
(i.e., replace the combined getCredential/listCredentialKeys check with a single
existence check using listCredentialKeys().includes(key)) so the flow only
proceeds when the key is actually stored on disk before prompting/deleting
(leave deleteCredential, askPrompt, args handling unchanged).
---
Nitpick comments:
In `@test/credentials.test.js`:
- Around line 67-82: The test description claims "leaves the file mode intact"
but never checks file permissions; add assertions using fs.statSync to capture
the credential file's permission mode (e.g., fs.statSync(<credential file
path>).mode & 0o777) before calling
credentials.deleteCredential("NVIDIA_API_KEY") and again after deletion and
assert they are equal; locate the credential file used by
importCredentialsModule/home in this test and place the two mode checks around
the deleteCredential call so the test verifies the file mode is unchanged.
🪄 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
Run ID: e2358e9c-31d6-4745-a8dd-d0bc7ea4ad70
📒 Files selected for processing (3)
bin/lib/credentials.jsbin/nemoclaw.jstest/credentials.test.js
✅ Actions performedReview triggered.
|
901f014 to
795da52
Compare
|
✅ Fixes already applied. CodeRabbit's findings from earlier are resolved:\n- ✅ bin/nemoclaw.js line 920: now uses |
|
✨ Thanks for submitting this pull request, which proposes a way to add a Possibly related open issues: |
cv
left a comment
There was a problem hiding this comment.
LGTM — security review PASS.
- Credential deletion uses atomic write (temp + rename) preserving 0o600 mode
- Only key names exposed, never values
- No path traversal risk — keys are JSON properties, not filesystem paths
- Interactive confirmation before deletion, --yes is opt-in
- Prototype pollution prevented with hasOwnProperty.call()
- Env-only keys correctly excluded from list (uses disk-only listCredentialKeys)
- Good test coverage
No concerns.
|
Several v0.0.10 PRs just merged, including changes to |
When a user enters an invalid API key during onboarding it gets saved to ~/.nemoclaw/credentials.json. On subsequent 'nemoclaw onboard' runs, ensureApiKey() and ensureNamedCredential() see the stored value and skip the prompt entirely, leaving the user stuck reusing the same bad key with no documented escape hatch other than hand-editing the JSON. Add a 'credentials' global subcommand: nemoclaw credentials list - list stored keys (no values) nemoclaw credentials reset <KEY> - remove a stored credential nemoclaw credentials reset <KEY> --yes - skip confirmation prompt After 'reset', the next 'nemoclaw onboard' run re-prompts for the key. Values are never printed by 'list' or by any error path. The existence check uses listCredentialKeys() only — getCredential() falls back to process.env, which would let an env-only key pass the check even though there is nothing on disk to delete. Adds three new tests covering deleteCredential and listCredentialKeys (round-trip with file-mode assertions, missing-file, and listing). Closes NVIDIA#1568 Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
795da52 to
73d19d9
Compare
|
@cv Thanks for the thorough security review! Rebased onto current main — clean linear history, CI should pick up a fresh run, cheers! |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bin/nemoclaw.js (1)
916-930:⚠️ Potential issue | 🟡 MinorHandle option-like/malformed
resetargs before key lookup.
nemoclaw credentials reset --yescurrently treats--yesas<KEY>and returns a misleading “No stored credential found”. Validate<KEY>as a positional argument (non-flag), and reject unknown extra flags for safer scripting behavior.Suggested patch
if (sub === "reset") { const key = args[1]; - if (!key) { + if (!key || key.startsWith("-")) { console.error(" Usage: nemoclaw credentials reset <KEY> [--yes]"); console.error(" Run 'nemoclaw credentials list' to see stored keys."); process.exit(1); } + const unknown = args.slice(2).filter((arg) => arg !== "--yes" && arg !== "-y"); + if (unknown.length > 0) { + console.error(` Unknown option(s): ${unknown.join(", ")}`); + console.error(" Usage: nemoclaw credentials reset <KEY> [--yes]"); + process.exit(1); + } // Only consult the persisted credentials file — getCredential() falls back🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 916 - 930, The reset command currently treats flags like "--yes" as the positional key (args[1]) causing misleading "No stored credential" errors; change the argument handling in the reset flow to first parse/validate that args[1] is a non-flag positional key (i.e., does not start with "-") and explicitly accept only the known flag(s) (--yes / -y) elsewhere (e.g., via args.includes("--yes") or args.includes("-y")); if args[1] is missing or is an option/flag, print the usage and exit, and if any unknown extra args/flags are present, reject them with an error before calling listCredentialKeys() so listCredentialKeys() and the subsequent existence check operate only on a validated key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bin/nemoclaw.js`:
- Around line 916-930: The reset command currently treats flags like "--yes" as
the positional key (args[1]) causing misleading "No stored credential" errors;
change the argument handling in the reset flow to first parse/validate that
args[1] is a non-flag positional key (i.e., does not start with "-") and
explicitly accept only the known flag(s) (--yes / -y) elsewhere (e.g., via
args.includes("--yes") or args.includes("-y")); if args[1] is missing or is an
option/flag, print the usage and exit, and if any unknown extra args/flags are
present, reject them with an error before calling listCredentialKeys() so
listCredentialKeys() and the subsequent existence check operate only on a
validated key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c83dbc64-70be-467b-be50-b3d7981b78fc
📒 Files selected for processing (3)
bin/nemoclaw.jssrc/lib/credentials.tstest/credentials.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/credentials.ts
- test/credentials.test.js
'nemoclaw credentials reset --yes' (with no key) previously treated '--yes' as <KEY> and reported 'No stored credential found for --yes'. Validate that <KEY> is a non-flag positional, and reject any trailing arguments other than --yes / -y so scripted use stays predictable. Addresses CodeRabbit feedback on NVIDIA#1597. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
## Summary - Document `nemoclaw credentials list` and `nemoclaw credentials reset` commands in commands reference (#1597) - Add `--dry-run` flag documentation for `policy-add` (#1276) - Update policy presets table: remove `docker` (#1647), add `brave` and `brew`, update HuggingFace endpoint (#1540) - Document `NEMOCLAW_LOCAL_INFERENCE_TIMEOUT` env var for local providers (#1620) - Document `NEMOCLAW_PROXY_HOST`/`NEMOCLAW_PROXY_PORT` env vars (#1563) - Add troubleshooting entries for Docker group permissions (#1614), sandbox survival after gateway restart (#1587), and proxy configuration - Regenerate `nemoclaw-user-*` skills from updated docs ## Test plan - [x] `make docs` builds without warnings - [x] All pre-commit and pre-push hooks pass - [ ] Verify rendered pages in docs site preview 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added `nemoclaw credentials list` command to display stored credential names * Added `nemoclaw credentials reset <KEY>` command with `--yes` flag to remove credentials * Added `--dry-run` flag for policy-add to preview endpoint changes * New policy presets: `brave` and `brew` * New configuration options: `NEMOCLAW_LOCAL_INFERENCE_TIMEOUT`, `NEMOCLAW_PROXY_HOST`, and `NEMOCLAW_PROXY_PORT` * **Documentation** * Expanded troubleshooting guides for Docker permissions, sandbox connectivity, local inference timeouts, and proxy configuration <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
deleteCredential(key)andlistCredentialKeys()helpers tobin/lib/credentials.jscredentialssubcommand:list,reset <KEY>,reset <KEY> --yesProblem
When a user enters an invalid API key during onboarding, the bad key is saved immediately to
~/.nemoclaw/credentials.json. On subsequentnemoclaw onboardruns,ensureApiKey()andensureNamedCredential()see the stored value and return early — the prompt never fires, validation fails again with the same error, and the user has no documented escape hatch other than hand-editing the JSON file.The recovery flow inside the onboard wizard does work if the user picks
retryafter validation fails, but #1568 specifically asks for either an automatic re-prompt on stored-credential validation failure, or a CLI command to clear a stored credential. This PR implements the CLI command (Option B in the issue) since it has the smaller blast radius and is decoupled from the onboard wizard.Behavior
After
reset, the nextnemoclaw onboardrun re-prompts for the key.listprints only key names, never values.resetconfirms by default (y/N);--yes/-yskips for scripting.listCredentialKeys()only —getCredential()was avoided because it falls back toprocess.env, which would let an env-only key pass the check even though there is nothing on disk to delete.deleteCredentialpreserves the existing 0o600 file permissions.false(not an error) when the file or key doesn't exist.Test plan
deleteCredentialandlistCredentialKeys0o600is preserved acrossdeleteCredentialcallscredentials help→ prints usagecredentials list(empty) → "No stored credentials."credentials list→ prints key name only, no valuecredentials reset KEY --yes→ removes the key, prints next-step hintcredentials reset MISSING --yes→ exits 1 with clear errorcredentials reset ENV_ONLY_KEY --yes(env var set, no disk entry) → exits 1, does not "delete" anythingnpx prettier --checkcleannpx eslintcleanCloses #1568
Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
New Features
nemoclaw credentialswithlist(show stored credential keys) andreset <KEY>(remove a stored credential, with optional confirmation).Tests