fix(credentials): host-side hardening of provider credential storage - #2554
Merged
Conversation
Stop persisting host-side provider credentials. saveCredential stages values in process.env only; getCredential reads from env only. Onboarding registers each value with the gateway via openshell provider upsert, and a one-time migration of any pre-fix credentials.json runs at the start of onboard and on rebuild preflight. The nemoclaw credentials list/reset subcommands now talk to OpenShell directly. ensureGithubToken no longer persists PATs; it defers to gh auth login for keychain-backed storage. Tests assert no host file is written, cover the legacy migration path (success, env precedence, corrupt input, no-file), and verify that credential values reach openshell via env without appearing in argv. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Rewrite docs/security/credential-storage.md so it reflects that the OpenShell gateway is the only system of record. Update commands, architecture, troubleshooting, and quickstart to remove references to ~/.nemoclaw/credentials.json. Regenerate the user-facing skills via scripts/docs-to-skills.py so the agent surface stays in sync. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
|
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:
📝 WalkthroughWalkthroughCredential handling moved from host-disk plaintext to in-process env staging and OpenShell gateway registration. Legacy Changes
Sequence DiagramsequenceDiagram
participant User as "User"
participant CLI as "NemoClaw CLI (process)"
participant Env as "Process Env"
participant FS as "Host Filesystem"
participant GW as "OpenShell Gateway"
participant API as "External API"
User->>CLI: run "nemoclaw onboard" (no creds in env)
CLI->>FS: check ~/.nemoclaw/credentials.json
alt legacy file present
FS-->>CLI: return legacy JSON (size-checked, allowlist)
CLI->>CLI: stageLegacyCredentialsToEnv() (no overwrite of set env)
CLI->>Env: set staged env vars
else no legacy file
FS-->>CLI: file not found
end
CLI->>Env: read provider creds from process.env
CLI->>GW: openshell provider create/update --credential <ENV_VAR>
GW-->>CLI: provider registered (gateway = system of record)
CLI->>FS: if staged keys migrated verbatim -> removeLegacyCredentialsFile() (secure wipe, unlink)
CLI->>API: request via OpenShell L7 proxy (credential injected at egress)
API-->>GW: response
GW-->>CLI: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/onboard.test.ts (1)
2449-2541:⚠️ Potential issue | 🟡 MinorCover the secure erase step, not just deletion.
This test proves that
credentials.jsonis unlinked, but it does not assert the zero-fill/overwrite step that the migration path is supposed to perform. A regression in the wipe logic would still pass here, so the security-sensitive part of the migration remains unverified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 2449 - 2541, The test only checks the credentials.json was deleted but not that it was securely zeroed first; modify the child script (the `script` string that invokes setupInference) to monkeypatch fs.unlinkSync (or wrap fs.unlink) so that when it is called for the legacy path (legacyFilePath) it first reads the file contents (fs.readFileSync) and asserts they are all zero bytes (or NULs) before delegating to the real unlink, then proceed to call setupInference as before; reference the child-side symbols `setupInference`, `legacyFilePath`, and the `fs` usage in the generated `script` to locate where to add the wrapper and the assertion.docs/reference/commands.md (2)
18-21:⚠️ Potential issue | 🟡 MinorUse the repo’s exact SPDX header text.
This file still carries
2025-2026, but the repo policy for touched Markdown sources requires the exact 2026-only header text. As per coding guidelines, "Every source file must include an SPDX license header:// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.and// SPDX-License-Identifier: Apache-2.0. Use#comments for shell scripts and HTML comments for Markdown."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/commands.md` around lines 18 - 21, Replace the existing SPDX header block in the Markdown file with the exact repo-required two-line SPDX header using an HTML comment, i.e., change the years "2025-2026" to the single year "2026" and ensure the two lines read exactly "SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved." and "SPDX-License-Identifier: Apache-2.0" inside the HTML comment; locate the current header comment at the top of docs/reference/commands.md and update it accordingly.
694-710:⚠️ Potential issue | 🟡 MinorUse a provider-name example for
credentials reset.The implementation treats the positional argument as an OpenShell provider name (
src/nemoclaw.ts, Line 1222), soNVIDIA_API_KEYis a misleading example here and is likely to send users down a failing path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/commands.md` around lines 694 - 710, The docs example for `nemoclaw credentials reset` is misleading because the CLI treats the positional argument as an OpenShell provider name (see command handling in src/nemoclaw.ts for `credentials reset`), so replace the `NVIDIA_API_KEY` example with a provider-name example (e.g., `nvidia`, `openai`, or whichever provider identifier your gateway uses) and update the short description to say “provider name” rather than “stored credential name” so the example and description match the CLI behavior.src/nemoclaw.ts (1)
1189-1248:⚠️ Potential issue | 🟠 MajorPin
credentials list/resetto the NemoClaw gateway.Both subcommands use bare
openshell provider ...calls without selecting the NemoClaw gateway first. If another gateway is active,listwill show the wrong providers andresetcan delete a provider from the wrong gateway. The codebase already establishes this pattern elsewhere (e.g., sandbox operations callrecoverNamedGatewayRuntime()before gateway-sensitive commands). Apply the same pattern here.Suggested fix
if (sub === "list") { + const recovery = await recoverNamedGatewayRuntime(); + if (!recovery.recovered) { + console.error(" Could not query the NemoClaw gateway. Is it running?"); + process.exit(1); + } const result = runOpenshell(["provider", "list", "--names"], {if (sub === "reset") { + const recovery = await recoverNamedGatewayRuntime(); + if (!recovery.recovered) { + console.error(" Could not query the NemoClaw gateway. Is it running?"); + process.exit(1); + } const result = runOpenshell(["provider", "delete", key], {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1189 - 1248, Both "credentials list" and "credentials reset" call runOpenshell(...) without selecting the NemoClaw gateway — call await recoverNamedGatewayRuntime() first to pin the gateway. Insert an awaited call to recoverNamedGatewayRuntime() before the runOpenshell([...], ...) invocation in the sub === "list" branch and likewise before the runOpenshell([...], ...) in the sub === "reset" branch so the subsequent runOpenshell and deleteCredential(key) operate against the NemoClaw gateway.
🧹 Nitpick comments (3)
docs/reference/architecture.md (1)
250-255: Move the gateway provider store out of the host-side table.This row describes OpenShell-managed credential storage, not host-side state. Relocating it to the gateway section would avoid implying that credentials live on the host.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/architecture.md` around lines 250 - 255, The table row labeled "OpenShell gateway provider store" incorrectly belongs in the host-side state table; update the docs by removing that row from the host-state section and adding an equivalent entry under the gateway/provider section (or create a "Gateway" subsection) with the same text about provider credentials being injected by the OpenShell L7 proxy and the link to Credential Storage, leaving the other rows (`~/.nemoclaw/sandboxes.json`, `~/.openclaw/openclaw.json`) untouched.docs/reference/commands.md (1)
440-440: Split this paragraph to one sentence per source line.Line 440 packs multiple sentences into one Markdown source line, which makes future doc diffs harder to review. As per coding guidelines, "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/commands.md` at line 440, The paragraph that begins "Pause a single messaging bridge (`telegram`, `discord`, or `slack`) without clearing its credentials." currently contains multiple sentences on one Markdown source line; split it so each sentence is on its own source line (e.g., separate the sentences about the channel being marked disabled, the sandbox being rebuilt so onboard skips registering the bridge, and the provider remaining registered so `channels start` restores the bridge) to comply with the "one sentence per line" guideline and improve future diffs.docs/security/credential-storage.md (1)
46-46: Split these paragraphs to one sentence per source line.Each of these lines contains multiple sentences, which violates the docs source-formatting rule. As per coding guidelines, "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
Also applies to: 116-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/security/credential-storage.md` at line 46, Split the multi-sentence source lines so each sentence is on its own line: locate the line containing "Both surface the provider names that the gateway holds credentials for. The values themselves cannot be read back from the CLI; this is a deliberate property of OpenShell." and break it into two lines (one for "Both surface the provider names that the gateway holds credentials for." and one for "The values themselves cannot be read back from the CLI; this is a deliberate property of OpenShell."), and apply the same one-sentence-per-line fix to the other occurrence referenced by the reviewer (the similar multi-sentence line at the later occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/security/credential-storage.md`:
- Around line 28-29: The paragraph incorrectly groups "nemoclaw config
rotate-token" with provider credential upserts; change the text to remove or
separate "nemoclaw config rotate-token" from the provider-credential lifecycle
and instead state that provider credentials are supplied via "nemoclaw onboard"
or as env vars and are registered with the OpenShell gateway using "openshell
provider create" / "openshell provider update", while "nemoclaw config
rotate-token" is a distinct sandbox/OpenClaw auth-token flow and should be
documented separately.
In `@src/lib/credentials.ts`:
- Around line 210-219: The migration loop in credentials.ts currently copies any
string-valued entry from parsed into process.env; restrict this to only
allowlisted keys by checking KNOWN_CREDENTIAL_ENV_KEYS before
normalizing/migrating: inside the for (const [key, value]...) loop, skip keys
not present in KNOWN_CREDENTIAL_ENV_KEYS (e.g., if
(!KNOWN_CREDENTIAL_ENV_KEYS.has(key)) continue), then proceed to call
normalizeCredentialValue, set process.env[key] only when absent, and push key
into migrated; this prevents arbitrary entries (like PATH or NODE_OPTIONS) from
being injected.
- Around line 152-176: secureUnlink currently follows symlinks because it uses
fs.statSync and fs.openSync; change it to use fs.lstatSync to detect symlinks
(stat.isSymbolicLink()) and if the path is a symlink, do not open or overwrite
the target — just unlink the symlink itself; for regular files, open with
fs.openSync using fs.constants.O_NOFOLLOW (and appropriate read/write flags) to
prevent following symlinks, then perform the zero-overwrite, fs.fsyncSync and
close, and finally unlink; update the secureUnlink function to handle
lstat/isSymbolicLink, use O_NOFOLLOW on openSync, and keep the existing
try/catch best-effort behavior.
In `@src/lib/onboard.ts`:
- Around line 673-680: The call to migrateLegacyCredentialsFile() inside
hydrateCredentialEnv() prematurely deletes the legacy
~/.nemoclaw/credentials.json before the gateway write occurs, risking data loss;
update the flow so hydrateCredentialEnv() only stages legacy values into
process.env (or call a new non-destructive helper, e.g.
stageLegacyCredentials()) and do NOT unlink the legacy file there, and move the
unlink/remove logic from migrateLegacyCredentialsFile() into the success path of
the gateway write (after setupInference() / createSandbox() completes their
provider upsert/writes); ensure getCredential(envName) still reads from
process.env so callers behave the same.
- Around line 3029-3031: The confirmation text in formatOnboardConfigSummary()
currently says the API key is "registered with the OpenShell gateway" even
though setupInference() hasn't run yet; change the apiKeyLine wording to
indicate the key is only staged (e.g., "staged for registration with the
OpenShell gateway" or "will be registered if you proceed") and update the
corresponding reset/cleanup message used later (the lines referenced around the
reset prompt) so it tells the user how to clear the staged credential rather
than resetting a gateway credential that was never created; make the same
wording change in both places where the message is constructed so the UI
consistently reflects "staged" state until setupInference() completes.
---
Outside diff comments:
In `@docs/reference/commands.md`:
- Around line 18-21: Replace the existing SPDX header block in the Markdown file
with the exact repo-required two-line SPDX header using an HTML comment, i.e.,
change the years "2025-2026" to the single year "2026" and ensure the two lines
read exactly "SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION &
AFFILIATES. All rights reserved." and "SPDX-License-Identifier: Apache-2.0"
inside the HTML comment; locate the current header comment at the top of
docs/reference/commands.md and update it accordingly.
- Around line 694-710: The docs example for `nemoclaw credentials reset` is
misleading because the CLI treats the positional argument as an OpenShell
provider name (see command handling in src/nemoclaw.ts for `credentials reset`),
so replace the `NVIDIA_API_KEY` example with a provider-name example (e.g.,
`nvidia`, `openai`, or whichever provider identifier your gateway uses) and
update the short description to say “provider name” rather than “stored
credential name” so the example and description match the CLI behavior.
In `@src/nemoclaw.ts`:
- Around line 1189-1248: Both "credentials list" and "credentials reset" call
runOpenshell(...) without selecting the NemoClaw gateway — call await
recoverNamedGatewayRuntime() first to pin the gateway. Insert an awaited call to
recoverNamedGatewayRuntime() before the runOpenshell([...], ...) invocation in
the sub === "list" branch and likewise before the runOpenshell([...], ...) in
the sub === "reset" branch so the subsequent runOpenshell and
deleteCredential(key) operate against the NemoClaw gateway.
In `@test/onboard.test.ts`:
- Around line 2449-2541: The test only checks the credentials.json was deleted
but not that it was securely zeroed first; modify the child script (the `script`
string that invokes setupInference) to monkeypatch fs.unlinkSync (or wrap
fs.unlink) so that when it is called for the legacy path (legacyFilePath) it
first reads the file contents (fs.readFileSync) and asserts they are all zero
bytes (or NULs) before delegating to the real unlink, then proceed to call
setupInference as before; reference the child-side symbols `setupInference`,
`legacyFilePath`, and the `fs` usage in the generated `script` to locate where
to add the wrapper and the assertion.
---
Nitpick comments:
In `@docs/reference/architecture.md`:
- Around line 250-255: The table row labeled "OpenShell gateway provider store"
incorrectly belongs in the host-side state table; update the docs by removing
that row from the host-state section and adding an equivalent entry under the
gateway/provider section (or create a "Gateway" subsection) with the same text
about provider credentials being injected by the OpenShell L7 proxy and the link
to Credential Storage, leaving the other rows (`~/.nemoclaw/sandboxes.json`,
`~/.openclaw/openclaw.json`) untouched.
In `@docs/reference/commands.md`:
- Line 440: The paragraph that begins "Pause a single messaging bridge
(`telegram`, `discord`, or `slack`) without clearing its credentials." currently
contains multiple sentences on one Markdown source line; split it so each
sentence is on its own source line (e.g., separate the sentences about the
channel being marked disabled, the sandbox being rebuilt so onboard skips
registering the bridge, and the provider remaining registered so `channels
start` restores the bridge) to comply with the "one sentence per line" guideline
and improve future diffs.
In `@docs/security/credential-storage.md`:
- Line 46: Split the multi-sentence source lines so each sentence is on its own
line: locate the line containing "Both surface the provider names that the
gateway holds credentials for. The values themselves cannot be read back from
the CLI; this is a deliberate property of OpenShell." and break it into two
lines (one for "Both surface the provider names that the gateway holds
credentials for." and one for "The values themselves cannot be read back from
the CLI; this is a deliberate property of OpenShell."), and apply the same
one-sentence-per-line fix to the other occurrence referenced by the reviewer
(the similar multi-sentence line at the later occurrence).
🪄 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: 3b469d5e-ba51-4857-9afd-2cf56e2dbfe9
📒 Files selected for processing (20)
.agents/skills/nemoclaw-user-configure-security/SKILL.md.agents/skills/nemoclaw-user-configure-security/references/credential-storage.md.agents/skills/nemoclaw-user-get-started/SKILL.md.agents/skills/nemoclaw-user-manage-policy/SKILL.md.agents/skills/nemoclaw-user-reference/references/architecture.md.agents/skills/nemoclaw-user-reference/references/commands.md.agents/skills/nemoclaw-user-reference/references/troubleshooting.mddocs/get-started/quickstart.mddocs/reference/architecture.mddocs/reference/commands.mddocs/reference/troubleshooting.mddocs/security/credential-storage.mdsrc/lib/credentials.tssrc/lib/onboard.tssrc/lib/sandbox-config.tssrc/nemoclaw.tstest/credentials.test.tstest/e2e/test-onboard-repair.shtest/e2e/test-onboard-resume.shtest/onboard.test.ts
- Split the legacy credentials.json migration into a non-destructive stageLegacyCredentialsToEnv() and a removeLegacyCredentialsFile() that runs only after a full successful onboard. An interrupted run can be retried without losing the user's only copy of their credentials. - Restrict legacy hydration to KNOWN_CREDENTIAL_ENV_KEYS so a stale or tampered credentials.json cannot inject PATH, NODE_OPTIONS, OPENSHELL_GATEWAY, or other unrelated variables into child processes. - Make secureUnlink symlink-safe: lstat the path, treat any symlink as a link-only unlink, and open regular files with O_NOFOLLOW so a planted symlink at ~/.nemoclaw/credentials.json cannot redirect the zero-fill onto an unrelated file. - Pin nemoclaw credentials list/reset to the NemoClaw gateway via recoverNamedGatewayRuntime() so a different active gateway cannot make us list or delete the wrong providers. - Reword the onboard preview and abort messaging so the credential is described as "staged for OpenShell gateway registration" until the upsert actually succeeds. Tests cover the allowlist filter, symlink-safety of removeLegacy, and the zero-fill step before unlink. The setupInference legacy-file test now asserts the file survives staging (only a successful onboard removes it). Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
- Drop the `nemoclaw config rotate-token` reference from docs/security/credential-storage.md; that command rotates a sandbox OpenClaw auth token, not a provider credential, and conflating the two flows obscured the lifecycle. - Move the OpenShell gateway provider store out of the host-side state table in docs/reference/architecture.md into its own section, since it is gateway-managed and not host-disk state. - Update the `nemoclaw credentials reset` example to use a provider name instead of a credential env var, matching the implementation. - Fix the SPDX header in docs/reference/commands.md to single-year 2026. - Split the `channels stop` paragraph onto separate sentence lines for reviewable diffs. - Regenerate user skills via scripts/docs-to-skills.py. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
… points Bring docstring coverage on the credentials module above the CodeRabbit 80% pre-merge threshold. Each exported function in src/lib/credentials.ts now carries a brief JSDoc describing its contract — env-only staging, legacy-file two-phase migration, prompt behavior, and the gh-CLI fallback path for GITHUB_TOKEN. Also lift hydrateCredentialEnv's block comment in src/lib/onboard.ts into a structured JSDoc. No behavior changes. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/onboard.test.ts (1)
2449-2549:⚠️ Potential issue | 🟡 MinorFix the migration comment.
hydrateCredentialEnv()only stages legacy credentials intoprocess.env; it does not deletecredentials.json. This comment describes the later cleanup step, so it will mislead readers about the test’s intent.Based on learnings,
hydrateCredentialEnv()stages legacy credentials non-destructively, andremoveLegacyCredentialsFile()performs the secure deletion only after a successful onboard.🛠️ Proposed fix
- // The new credentials module migrates it into process.env and securely deletes the file - // the first time hydrateCredentialEnv runs. + // The new credentials module stages it into process.env without deleting + // the file; cleanup happens after a successful onboard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 2449 - 2549, Update the misleading comment that says the new credentials module "securely deletes the file the first time hydrateCredentialEnv runs" to accurately state that hydrateCredentialEnv only stages legacy credentials into process.env non-destructively; the secure deletion is performed later by removeLegacyCredentialsFile (invoked after a successful onboard). Locate the comment near the test around setupInference/hydrateCredentialEnv and replace the wording to mention staging-only behavior and that removeLegacyCredentialsFile handles deletion post-onboard.docs/reference/architecture.md (1)
18-23:⚠️ Potential issue | 🟡 MinorAlign the page header with the docs contract.
The top matter still violates the docs style guide: the H1 does not match
title.page, and the SPDX copyright line uses2025-2026instead of the repo’s 2026 header. Please update both to keep the page metadata consistent.As per coding guidelines, docs/**/*.md pages require the H1 to match
title.page, and Markdown source files must carry the repo SPDX header.♻️ Proposed fix
- SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. @@ -# Architecture +# NemoClaw Architecture: Plugin, Blueprint, and Sandbox Structure🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/architecture.md` around lines 18 - 23, Update the SPDX header year to the repo standard (use 2026 instead of "2025-2026") and make the page H1 match the docs contract by ensuring the top-level heading equals title.page; specifically replace the current SPDX line with the repo's exact SPDX header (year 2026) and change the H1 "# Architecture" so it either matches the value of title.page in the file's frontmatter or update title.page to "Architecture" so both are identical.src/lib/onboard.ts (1)
6489-6507:⚠️ Potential issue | 🟠 MajorRegister lock cleanup before legacy staging.
The lock is acquired on Line 6473, but
stageLegacyCredentialsToEnv()is called beforereleaseOnboardLockis registered and before thetry/finally. If staging throws, a stale lock can be left behind.🔧 Proposed fix
- // Stage any pre-fix plaintext credentials.json into process.env so the - // provider upserts later in this run can pick the values up. The file is - // NOT removed here — the secure unlink runs only after onboarding - // completes successfully, so an interrupted run can be retried. - const stagedLegacyKeys = stageLegacyCredentialsToEnv(); - if (stagedLegacyKeys.length > 0) { - console.error( - ` Staged ${String(stagedLegacyKeys.length)} legacy credential(s) for migration to the OpenShell gateway.`, - ); - } + let stagedLegacyKeys: string[] = []; let lockReleased = false; const releaseOnboardLock = () => { if (lockReleased) return; lockReleased = true; onboardSession.releaseOnboardLock(); }; process.once("exit", releaseOnboardLock); try { + // Stage any pre-fix plaintext credentials.json into process.env so the + // provider upserts later in this run can pick the values up. The file is + // NOT removed here — the secure unlink runs only after onboarding + // completes successfully, so an interrupted run can be retried. + stagedLegacyKeys = stageLegacyCredentialsToEnv(); + if (stagedLegacyKeys.length > 0) { + console.error( + ` Staged ${String(stagedLegacyKeys.length)} legacy credential(s) for migration to the OpenShell gateway.`, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 6489 - 6507, The staging call can throw before the onboard lock cleanup is registered, leaving a stale lock; declare lockReleased/releaseOnboardLock and call process.once("exit", releaseOnboardLock) (and enter the try/finally that calls onboardSession.releaseOnboardLock() in finally) immediately after acquiring the lock and before calling stageLegacyCredentialsToEnv(); then run stageLegacyCredentialsToEnv() inside that try so any thrown error still triggers releaseOnboardLock/onboardSession.releaseOnboardLock(). Use the existing symbols lockReleased, releaseOnboardLock, process.once("exit", ...), onboardSession.releaseOnboardLock, and stageLegacyCredentialsToEnv to locate and reorder the code.
🧹 Nitpick comments (1)
src/lib/credentials.ts (1)
94-96: Comment references a non-existent helper name.This comment mentions
migrateLegacyCredentialsFile(), but the implemented helpers arestageLegacyCredentialsToEnvandremoveLegacyCredentialsFile.🧹 Suggested wording fix
-// Path of the pre-migration plaintext credentials file. Retained only so -// migrateLegacyCredentialsFile() can find and securely delete it. New code -// must NOT write to this path; the gateway is the system of record. +// Path of the pre-migration plaintext credentials file. Retained only for +// legacy migration helpers (`stageLegacyCredentialsToEnv` and +// `removeLegacyCredentialsFile`). New code must NOT write to this path; the +// gateway is the system of record.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/credentials.ts` around lines 94 - 96, Update the misleading comment that references migrateLegacyCredentialsFile() to instead mention the actual helper functions stageLegacyCredentialsToEnv and removeLegacyCredentialsFile; keep the rest of the guidance (path retained only so those helpers can find and securely delete it, and new code must NOT write to this path because the gateway is the system of record) but replace the nonexistent function name with the two real symbols stageLegacyCredentialsToEnv and removeLegacyCredentialsFile to make the comment accurate.
🤖 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/onboard.ts`:
- Around line 6980-6986: The comment says legacy credentials are removed
unconditionally but the code only calls removeLegacyCredentialsFile() when
stagedLegacyKeys.length > 0; make behavior and comment consistent by removing
the conditional and calling removeLegacyCredentialsFile() unconditionally (i.e.,
delete the if (stagedLegacyKeys.length > 0) guard so
removeLegacyCredentialsFile() runs regardless), referencing the stagedLegacyKeys
variable and the removeLegacyCredentialsFile() call in onboard.ts.
---
Outside diff comments:
In `@docs/reference/architecture.md`:
- Around line 18-23: Update the SPDX header year to the repo standard (use 2026
instead of "2025-2026") and make the page H1 match the docs contract by ensuring
the top-level heading equals title.page; specifically replace the current SPDX
line with the repo's exact SPDX header (year 2026) and change the H1 "#
Architecture" so it either matches the value of title.page in the file's
frontmatter or update title.page to "Architecture" so both are identical.
In `@src/lib/onboard.ts`:
- Around line 6489-6507: The staging call can throw before the onboard lock
cleanup is registered, leaving a stale lock; declare
lockReleased/releaseOnboardLock and call process.once("exit",
releaseOnboardLock) (and enter the try/finally that calls
onboardSession.releaseOnboardLock() in finally) immediately after acquiring the
lock and before calling stageLegacyCredentialsToEnv(); then run
stageLegacyCredentialsToEnv() inside that try so any thrown error still triggers
releaseOnboardLock/onboardSession.releaseOnboardLock(). Use the existing symbols
lockReleased, releaseOnboardLock, process.once("exit", ...),
onboardSession.releaseOnboardLock, and stageLegacyCredentialsToEnv to locate and
reorder the code.
In `@test/onboard.test.ts`:
- Around line 2449-2549: Update the misleading comment that says the new
credentials module "securely deletes the file the first time
hydrateCredentialEnv runs" to accurately state that hydrateCredentialEnv only
stages legacy credentials into process.env non-destructively; the secure
deletion is performed later by removeLegacyCredentialsFile (invoked after a
successful onboard). Locate the comment near the test around
setupInference/hydrateCredentialEnv and replace the wording to mention
staging-only behavior and that removeLegacyCredentialsFile handles deletion
post-onboard.
---
Nitpick comments:
In `@src/lib/credentials.ts`:
- Around line 94-96: Update the misleading comment that references
migrateLegacyCredentialsFile() to instead mention the actual helper functions
stageLegacyCredentialsToEnv and removeLegacyCredentialsFile; keep the rest of
the guidance (path retained only so those helpers can find and securely delete
it, and new code must NOT write to this path because the gateway is the system
of record) but replace the nonexistent function name with the two real symbols
stageLegacyCredentialsToEnv and removeLegacyCredentialsFile to make the comment
accurate.
🪄 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: 44c6f0b8-4d38-4b05-bf94-82c3b9ce7d1a
📒 Files selected for processing (11)
.agents/skills/nemoclaw-user-configure-security/references/credential-storage.md.agents/skills/nemoclaw-user-reference/references/architecture.md.agents/skills/nemoclaw-user-reference/references/commands.mddocs/reference/architecture.mddocs/reference/commands.mddocs/security/credential-storage.mdsrc/lib/credentials.tssrc/lib/onboard.tssrc/nemoclaw.tstest/credentials.test.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .agents/skills/nemoclaw-user-reference/references/architecture.md
- .agents/skills/nemoclaw-user-reference/references/commands.md
- docs/reference/commands.md
- docs/security/credential-storage.md
Contributor
There was a problem hiding this comment.
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/credentials.ts`:
- Around line 265-268: In removeLegacyCredentialsFile, drop the
fs.existsSync(legacyFile) pre-check and just call secureUnlink(legacyFile)
directly: locate the removeLegacyCredentialsFile function (which uses
getCredsFile() to compute legacyFile) and replace the conditional guard with a
single call to secureUnlink(legacyFile) so lstat-based handling in secureUnlink
is used (avoiding existsSync’s symlink/dangling-link behavior).
- Around line 247-257: The staged list currently includes keys even when the
legacy value is skipped due to an existing process.env, causing false positives
for stagedLegacyKeys; update the loop in the credentials import function so that
staged.push(key) only runs when you actually set the env (i.e., inside the
branch where you assign process.env[key] = normalized), leaving it out when you
continue because process.env[key] already existed; keep using
normalizeCredentialValue and return staged.sort() as before so only
truly-migrated keys are reported.
🪄 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: 6c83f87e-6e2c-4925-9a03-6a4b6eafcd11
📒 Files selected for processing (2)
src/lib/credentials.tssrc/lib/onboard.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard.ts
Adds test/e2e/test-credential-migration.sh to the nightly E2E suite to
exercise the host-side credential storage hardening end-to-end:
1. Pre-seed ~/.nemoclaw/credentials.json with the real NVIDIA_API_KEY
plus a tampered, non-allowlisted entry. Run nemoclaw onboard with
NVIDIA_API_KEY unset in the environment so the only credential
source is the legacy file.
2. Assert onboard succeeds, the migration notice is emitted to
stderr, and the legacy file is securely removed.
3. Assert the value reaches the OpenShell gateway via openshell
provider list --names, and the tampered keys did not become
gateway providers.
4. Run nemoclaw credentials list and confirm it surfaces the
gateway-registered providers without re-creating credentials.json.
5. Plant a symlink at the credentials path pointing at an unrelated
victim file; invoke removeLegacyCredentialsFile() directly via
the compiled module; assert the symlink is unlinked but the
target file is intact (validates lstat + O_NOFOLLOW path).
Wires the new job into nightly-e2e.yaml alongside the other
credential-adjacent jobs and into notify-on-failure's needs list.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/test-credential-migration.sh (1)
66-70: Remove or use the unusedskip()helper.
skip()is currently dead code; either add a real skip path or removeskip()/SKIPto keep this script lean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-credential-migration.sh` around lines 66 - 70, The helper skip() and its counter SKIP are unused; either remove the skip() function and any SKIP references (and stop incrementing TOTAL there) to clean up the script, or implement a real skip path that calls skip() where tests are conditionally skipped (ensuring SKIP and TOTAL are declared/initialized and printed in the summary). Locate the skip() function and SKIP/TOTAL usage in the test/e2e/test-credential-migration.sh script and choose one of the two changes: delete the function and SKIP increments, or wire skip() into actual conditional branches that mark tests as skipped and update the summary output accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/test-credential-migration.sh`:
- Line 189: The test currently masks CLI failures by appending "|| true" to the
openshell calls (the PROVIDERS_OUT capture and the similar command at line 221),
so change the test to let the command fail instead of swallowing errors: remove
"|| true" from the openshell invocations that set PROVIDERS_OUT (and the other
capture), capture the command's exit status and assert it is zero or fail the
test immediately if non-zero, then use the normal stdout/stderr variables for
assertions; reference the PROVIDERS_OUT assignment (openshell -g nemoclaw
provider list) and the other openshell capture at line 221 when making this
change so downstream assertions operate on valid command output rather than
error text.
---
Nitpick comments:
In `@test/e2e/test-credential-migration.sh`:
- Around line 66-70: The helper skip() and its counter SKIP are unused; either
remove the skip() function and any SKIP references (and stop incrementing TOTAL
there) to clean up the script, or implement a real skip path that calls skip()
where tests are conditionally skipped (ensuring SKIP and TOTAL are
declared/initialized and printed in the summary). Locate the skip() function and
SKIP/TOTAL usage in the test/e2e/test-credential-migration.sh script and choose
one of the two changes: delete the function and SKIP increments, or wire skip()
into actual conditional branches that mark tests as skipped and update the
summary output accordingly.
🪄 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: 02d1eede-5775-4a23-9fe0-3108d0008a2c
📒 Files selected for processing (2)
.github/workflows/nightly-e2e.yamltest/e2e/test-credential-migration.sh
The credentials-reset heading in docs/reference/commands.md was renamed from "<KEY>" to "<PROVIDER>" to match the implementation, which invalidated the auto-generated anchor "nemoclaw-credentials-reset-key" that quickstart.md still pointed at. Sphinx's nitpicky warnings-as-errors mode caught it and failed the docs build. Update the cross-reference to "nemoclaw-credentials-reset-provider", and revise the surrounding example to use a provider name instead of an env-var name. Regenerate user skills. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
The zero-fill verification test mutated `bytesAtUnlink: Buffer | null`
from inside a vi.spyOn callback. TypeScript narrowed the slot to `null`
permanently because the only assignment is inside a closure, so the
post-call `.length` and `.every` accesses tripped TS2339/TS7006 in the
`checks` PR job.
Hold the capture on a holder object (`{ bytes: Buffer | null }`) so the
slot type is preserved across the closure. Typecheck and tests pass
locally.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/get-started/quickstart.md (1)
334-334: Use active voice in this sentence.This line uses passive voice (“was entered”). Please rewrite in active voice.
Suggested edit
-If a provider credential was entered incorrectly during onboarding, clear the gateway-registered value and re-enter it on the next onboard run: +If you entered a provider credential incorrectly during onboarding, clear the gateway-registered value and re-enter it on the next onboard run:As per coding guidelines, "Active voice required. Flag passive constructions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/get-started/quickstart.md` at line 334, The sentence uses passive voice ("was entered"); rewrite it in active voice by making the subject perform the action — e.g., change "If a provider credential was entered incorrectly during onboarding, clear the gateway-registered value and re-enter it on the next onboard run:" to an active construction such as "If you entered a provider credential incorrectly during onboarding, clear the gateway-registered value and re-enter it on the next onboard run." Update the sentence in docs/get-started/quickstart.md accordingly.test/credentials.test.ts (1)
12-20: Tighten the module export type-guard to fail earlier.
isCredentialsModulevalidates only part of what this file later uses. Consider checking all invoked exports so import failures surface at one clear boundary.♻️ Suggested refactor
function isCredentialsModule(value: object | null): value is CredentialsModule { return ( value !== null && typeof Reflect.get(value, "loadCredentials") === "function" && typeof Reflect.get(value, "getCredential") === "function" && typeof Reflect.get(value, "saveCredential") === "function" && + typeof Reflect.get(value, "deleteCredential") === "function" && + typeof Reflect.get(value, "listCredentialKeys") === "function" && + typeof Reflect.get(value, "normalizeCredentialValue") === "function" && typeof Reflect.get(value, "stageLegacyCredentialsToEnv") === "function" && typeof Reflect.get(value, "removeLegacyCredentialsFile") === "function" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credentials.test.ts` around lines 12 - 20, The current isCredentialsModule type-guard only checks a subset of exports and can let missing imports slip through; update isCredentialsModule to assert the presence and types of every export this test file later invokes (start by adding explicit typeof checks for each function the tests call — e.g. loadCredentials, getCredential, saveCredential, stageLegacyCredentialsToEnv, removeLegacyCredentialsFile — and any additional helpers referenced elsewhere in the file) so import failures surface immediately; modify the guard (the isCredentialsModule function) to include those extra property checks using Reflect.get or in-operator + typeof so the returned type predicate truly matches the test usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/get-started/quickstart.md`:
- Around line 337-342: The docs Quickstart uses the placeholder <PROVIDER> but
the CLI help and usage for the command prints <KEY>, causing a mismatch; update
the Quickstart text and examples (the two occurrences of "nemoclaw credentials
reset <PROVIDER>" and any surrounding explanatory text) to use <KEY> instead so
they match the actual CLI output for the nemoclaw credentials reset command.
In `@test/credentials.test.ts`:
- Around line 190-215: The test currently asserts process.env.NODE_OPTIONS and
process.env.OPENSHELL_GATEWAY are undefined which is brittle; before writing the
legacy file capture the originals (like how originalPath is captured) into local
variables (e.g., originalNodeOptions, originalOpenshellGateway) and after
calling importCredentialsModule(...) and stageLegacyCredentialsToEnv() assert
that process.env.NODE_OPTIONS === originalNodeOptions and
process.env.OPENSHELL_GATEWAY === originalOpenshellGateway so the test verifies
those env vars are unchanged rather than forcing undefined; update the
assertions in the test block that calls stageLegacyCredentialsToEnv()
accordingly.
---
Nitpick comments:
In `@docs/get-started/quickstart.md`:
- Line 334: The sentence uses passive voice ("was entered"); rewrite it in
active voice by making the subject perform the action — e.g., change "If a
provider credential was entered incorrectly during onboarding, clear the
gateway-registered value and re-enter it on the next onboard run:" to an active
construction such as "If you entered a provider credential incorrectly during
onboarding, clear the gateway-registered value and re-enter it on the next
onboard run." Update the sentence in docs/get-started/quickstart.md accordingly.
In `@test/credentials.test.ts`:
- Around line 12-20: The current isCredentialsModule type-guard only checks a
subset of exports and can let missing imports slip through; update
isCredentialsModule to assert the presence and types of every export this test
file later invokes (start by adding explicit typeof checks for each function the
tests call — e.g. loadCredentials, getCredential, saveCredential,
stageLegacyCredentialsToEnv, removeLegacyCredentialsFile — and any additional
helpers referenced elsewhere in the file) so import failures surface
immediately; modify the guard (the isCredentialsModule function) to include
those extra property checks using Reflect.get or in-operator + typeof so the
returned type predicate truly matches the test usage.
🪄 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: 3886d891-f7f3-415b-ba03-2bd39b27fda5
📒 Files selected for processing (3)
.agents/skills/nemoclaw-user-get-started/SKILL.mddocs/get-started/quickstart.mdtest/credentials.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/skills/nemoclaw-user-get-started/SKILL.md
…ay-only-signed Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
The PR-checks `checks` job runs shellcheck against changed files and
flagged two warnings on the new script:
- SC2329: skip() was defined but never invoked in this script.
The other E2E scripts in the repo carry the same skip helper, but
they actually call it from at least one branch. Drop it here since
no phase emits a SKIP — the FAIL/PASS pair is enough.
- SC2001: `echo "$VAR" | sed 's/^/ /'` is the textbook anti-pattern
shellcheck calls out. Replace the indent operation with a small
`indent` helper that pipes through awk, used at the two diagnostic
print sites.
Drop SKIP from the summary block since the helper is gone. Verified
clean locally with `shellcheck test/e2e/test-credential-migration.sh`.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
6987-6993:⚠️ Potential issue | 🟡 MinorComment and cleanup behavior are still inconsistent.
The comment says legacy file removal is unconditional, but deletion still runs only when
stagedLegacyKeys.length > 0. Please align one to the other.✏️ Comment-only alignment
- // Onboarding finished successfully and the gateway holds the migrated - // credentials. Now it is safe to securely delete the legacy plaintext - // ~/.nemoclaw/credentials.json. Done unconditionally so a long-stale - // file from a previous machine state is also cleaned up. + // Onboarding finished successfully and the gateway holds migrated + // credentials for keys staged in this run. It is now safe to securely + // delete the legacy plaintext ~/.nemoclaw/credentials.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 6987 - 6993, The comment claims legacy credentials file removal is unconditional but the code only calls removeLegacyCredentialsFile() when stagedLegacyKeys.length > 0; make the behavior and comment consistent by either removing the conditional to always call removeLegacyCredentialsFile() or updating the comment to state deletion happens only when stagedLegacyKeys exists. Locate the conditional around stagedLegacyKeys and removeLegacyCredentialsFile() and either delete the if-check so removeLegacyCredentialsFile() is invoked unconditionally, or adjust the comment text to reflect the conditional deletion.
🤖 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/onboard.ts`:
- Around line 686-689: hydrateCredentialEnv currently always calls
stageLegacyCredentialsToEnv which unconditionally writes legacy values into
process.env and can clobber a freshly entered credential; change the flow so you
do not overwrite an already-present credential: in hydrateCredentialEnv check
process.env[envName] (or call getCredential first) and if a value exists return
it immediately without calling stageLegacyCredentialsToEnv, otherwise call
stageLegacyCredentialsToEnv and then return getCredential(envName);
alternatively, modify stageLegacyCredentialsToEnv to only set process.env keys
that are undefined so it never overwrites existing env vars (apply this change
where stageLegacyCredentialsToEnv and getCredential are used).
---
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 6987-6993: The comment claims legacy credentials file removal is
unconditional but the code only calls removeLegacyCredentialsFile() when
stagedLegacyKeys.length > 0; make the behavior and comment consistent by either
removing the conditional to always call removeLegacyCredentialsFile() or
updating the comment to state deletion happens only when stagedLegacyKeys
exists. Locate the conditional around stagedLegacyKeys and
removeLegacyCredentialsFile() and either delete the if-check so
removeLegacyCredentialsFile() is invoked unconditionally, or adjust the comment
text to reflect the conditional deletion.
🪄 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: 5fded1b8-450b-4703-bd8d-061ef93420e9
📒 Files selected for processing (4)
.agents/skills/nemoclaw-user-get-started/SKILL.mddocs/get-started/quickstart.mdsrc/lib/onboard.tstest/credentials.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/get-started/quickstart.md
- test/credentials.test.ts
…eview Five concerns from the latest CodeRabbit review: - stageLegacyCredentialsToEnv() previously pushed each allowlisted key into the `staged` return array even when the legacy value was skipped because process.env already held a user-supplied value. Onboard uses `stagedLegacyKeys.length > 0` to decide whether to delete the legacy file, so the false positives could unlink credentials we never actually migrated. Only push to `staged` inside the branch that actually sets process.env. - removeLegacyCredentialsFile() guarded the call to secureUnlink with fs.existsSync, which follows symlinks and returns false for dangling links. A planted dangling symlink at ~/.nemoclaw/credentials.json would have bypassed cleanup. secureUnlink already lstat's and tolerates missing files, so drop the redundant pre-check. - hydrateCredentialEnv() called stageLegacyCredentialsToEnv() unconditionally. Although the staging helper does not overwrite an existing env entry, we can short-circuit the file read entirely when the env already carries a value, which both removes redundant I/O on the hot path and makes the "do not shadow a freshly-entered credential" intent explicit at the call site. - The comment above the post-onboard removeLegacyCredentialsFile() call said deletion was "unconditional" while the code guarded it on stagedLegacyKeys.length > 0. Realign the comment to describe the conditional, since the conditional itself is correct (now that staged tracking ignores already-set env entries). - test/e2e/test-credential-migration.sh masked CLI failures with `|| true` when capturing `openshell provider list --names` and `nemoclaw credentials list`, so a real failure became silent and downstream grep assertions would run against error text. Capture exit status explicitly and fail the test with the captured stderr on a non-zero return. Add a regression assertion in the credentials test for the staged-array contract: when the env already holds a value, the legacy value is skipped AND the staged array stays empty so onboard does not unlink the file. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…ned' into fix/credentials-gateway-only-signed
…able The interactive prompt and the credential-storage doc both said the GitHub token would be stored in "the system keychain" as if that were the only outcome. On hosts where no keychain is reachable — CI runners, headless launches, WSL without a session bus, macOS contexts where Keychain access is blocked — `gh auth login` falls back to a gh-managed file under `~/.config/gh/`. NemoClaw treats both backends identically; `gh auth token` returns the value without caring which backend stored it. Update the box copy in `ensureGithubToken()` and the GitHub Tokens section of docs/security/credential-storage.md so they describe the actual behavior — keychain when reachable, otherwise the gh-managed file — without overpromising on platforms where the keychain path isn't available. No behavior changes; tests and typecheck pass. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/credentials.test.ts (1)
190-214:⚠️ Potential issue | 🟡 MinorAvoid brittle
undefinedassertions for inherited env vars.Line 213 and Line 214 can fail when the test runner already exports
NODE_OPTIONSorOPENSHELL_GATEWAY. Capture their original values likePATHand assert they are unchanged instead of forcingundefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credentials.test.ts` around lines 190 - 214, The test currently asserts process.env.NODE_OPTIONS and process.env.OPENSHELL_GATEWAY are undefined which is brittle; before writing the legacy file capture original values (like you do for PATH) into variables (e.g., originalNodeOptions, originalOpenShellGateway), call importCredentialsModule/home and stageLegacyCredentialsToEnv as before, and then assert process.env.NODE_OPTIONS === originalNodeOptions and process.env.OPENSHELL_GATEWAY === originalOpenShellGateway so the test verifies those env vars were left unchanged rather than forcing undefined; reference the test's use of importCredentialsModule and the stageLegacyCredentialsToEnv() call to locate where to add the captures and replace the undefined assertions.
🧹 Nitpick comments (3)
docs/get-started/quickstart.md (1)
334-334: Use active voice in this instruction.Line 334 uses passive voice (“was entered incorrectly”).
Please rewrite to active voice (for example, “If you entered a provider credential incorrectly ...”).As per coding guidelines: “Active voice required. Flag passive constructions.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/get-started/quickstart.md` at line 334, Replace the passive sentence "If a provider credential was entered incorrectly during onboarding, clear the gateway-registered value and re-enter it on the next onboard run:" with an active-voice version that addresses the reader, e.g., "If you entered a provider credential incorrectly during onboarding, clear the gateway-registered value and re-enter it on the next onboard run." Update the exact sentence in the Quickstart text so the wording uses "you entered" instead of "was entered."docs/reference/commands.md (2)
457-459: Use active voice for thechannels stopflow.Lines 458-459 describe the action passively (
is marked disabled,is rebuilt). Please make the CLI the actor here so the workflow reads directly.As per coding guidelines, "Active voice required. Flag passive constructions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/commands.md` around lines 457 - 459, Rewrite the passive descriptions for the `channels stop` flow into active voice: change "The channel is marked disabled in the per-sandbox registry, and the sandbox is rebuilt so the onboard step skips registering the bridge with the gateway" to sentences where the CLI performs the actions (e.g., "The CLI marks the channel disabled in the per-sandbox registry and rebuilds the sandbox so the onboard step skips registering the bridge with the gateway"), and ensure consistency with `channels start` wording and references to the `onboard` step and provider registration with the OpenShell gateway.
84-84: Split this into one sentence per line and rewrite it in active voice.Line 84 currently packs two sentences onto one source line and uses passive phrasing (
are registered,never persisted), which conflicts with the docs style guide.As per coding guidelines, "Active voice required. Flag passive constructions." and "One sentence per line in source (makes diffs readable)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/commands.md` at line 84, Rewrite the single source line into two separate lines and convert both clauses to active voice: first line should say "OpenShell registers credentials with the gateway." and second line should say "The gateway does not persist credentials to the host disk; see [Credential Storage](../security/credential-storage.md) for details on inspection, rotation, and migration from earlier releases." Ensure each sentence is on its own source line and preserve the existing link text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/commands.md`:
- Around line 722-729: Docs currently use `<PROVIDER>` but the CLI help/usage
(in src/nemoclaw.ts) prints "Usage: nemoclaw credentials reset <KEY> [--yes]"
and calls the provider argument KEY; make them consistent by reverting the docs
text to use `<KEY>` (or alternatively update the usage string in src/nemoclaw.ts
to `<PROVIDER>`). Locate the command/usage declaration for the "credentials
reset" command in src/nemoclaw.ts (the code that emits "Usage: nemoclaw
credentials reset <KEY> [--yes]") and either change the docs to `<KEY>` or
update that usage string and any help text to `<PROVIDER>` so both help output
and documentation match.
In `@src/lib/onboard.ts`:
- Around line 6502-6511: stageLegacyCredentialsToEnv() currently stages keys and
removeLegacyCredentialsFile() is called based on stagedLegacyKeys; instead,
change the cleanup to run only for the subset actually registered with the
gateway: after the provider upsert/registration call(s) (the function that
performs the OpenShell upsert/registration — e.g., the provider upsert or
registerProviders routine), capture and return the list of credentials that were
successfully upserted (call it registeredLegacyKeys), then only invoke
removeLegacyCredentialsFile() (or add an API to remove specific keys) when
registeredLegacyKeys.length > 0 and/or when registeredLegacyKeys matches
stagedLegacyKeys subset so you only delete legacy secrets that were actually
migrated; update any other similar call sites (including the other occurrence
referenced around the 6993–7000 area) to use the registered subset instead of
stagedLegacyKeys.
---
Duplicate comments:
In `@test/credentials.test.ts`:
- Around line 190-214: The test currently asserts process.env.NODE_OPTIONS and
process.env.OPENSHELL_GATEWAY are undefined which is brittle; before writing the
legacy file capture original values (like you do for PATH) into variables (e.g.,
originalNodeOptions, originalOpenShellGateway), call
importCredentialsModule/home and stageLegacyCredentialsToEnv as before, and then
assert process.env.NODE_OPTIONS === originalNodeOptions and
process.env.OPENSHELL_GATEWAY === originalOpenShellGateway so the test verifies
those env vars were left unchanged rather than forcing undefined; reference the
test's use of importCredentialsModule and the stageLegacyCredentialsToEnv() call
to locate where to add the captures and replace the undefined assertions.
---
Nitpick comments:
In `@docs/get-started/quickstart.md`:
- Line 334: Replace the passive sentence "If a provider credential was entered
incorrectly during onboarding, clear the gateway-registered value and re-enter
it on the next onboard run:" with an active-voice version that addresses the
reader, e.g., "If you entered a provider credential incorrectly during
onboarding, clear the gateway-registered value and re-enter it on the next
onboard run." Update the exact sentence in the Quickstart text so the wording
uses "you entered" instead of "was entered."
In `@docs/reference/commands.md`:
- Around line 457-459: Rewrite the passive descriptions for the `channels stop`
flow into active voice: change "The channel is marked disabled in the
per-sandbox registry, and the sandbox is rebuilt so the onboard step skips
registering the bridge with the gateway" to sentences where the CLI performs the
actions (e.g., "The CLI marks the channel disabled in the per-sandbox registry
and rebuilds the sandbox so the onboard step skips registering the bridge with
the gateway"), and ensure consistency with `channels start` wording and
references to the `onboard` step and provider registration with the OpenShell
gateway.
- Line 84: Rewrite the single source line into two separate lines and convert
both clauses to active voice: first line should say "OpenShell registers
credentials with the gateway." and second line should say "The gateway does not
persist credentials to the host disk; see [Credential
Storage](../security/credential-storage.md) for details on inspection, rotation,
and migration from earlier releases." Ensure each sentence is on its own source
line and preserve the existing link text.
🪄 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: 52f0dded-a713-4cfc-8f12-a0bd0c666d08
📒 Files selected for processing (10)
.agents/skills/nemoclaw-user-get-started/SKILL.md.agents/skills/nemoclaw-user-reference/references/commands.md.agents/skills/nemoclaw-user-reference/references/troubleshooting.mddocs/get-started/quickstart.mddocs/reference/commands.mddocs/reference/troubleshooting.mdsrc/lib/credentials.tssrc/lib/onboard.tstest/credentials.test.tstest/e2e/test-credential-migration.sh
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/nemoclaw-user-reference/references/troubleshooting.md
- docs/reference/troubleshooting.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/test-credential-migration.sh
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/security/credential-storage.md (3)
48-48: Split into one sentence per line.This line contains two sentences, which violates the formatting rule for readable diffs. Additionally, "cannot be read back" uses passive voice.
-Both surface the provider names that the gateway holds credentials for. The values themselves cannot be read back from the CLI; this is a deliberate property of OpenShell. +Both surface the provider names that the gateway holds credentials for. +The CLI cannot read the values themselves back; this is a deliberate property of OpenShell.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/security/credential-storage.md` at line 48, Break the single line into two lines with one sentence per line and change the passive phrasing to active voice: rewrite the text as two sentences such as "The CLI lists the provider names for which the gateway holds credentials." and "The CLI does not display the credential values; this is a deliberate property of OpenShell." Ensure each sentence sits on its own line in docs/security/credential-storage.md.
50-51: Minor: Passive voice."is created" uses passive voice. Consider: "NemoClaw creates that directory with mode
0700and stores no credential material in it."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/security/credential-storage.md` around lines 50 - 51, Reword the passive sentence to active voice: replace "That directory is created with mode `0700` and contains no credential material." with an active construction such as "NemoClaw creates that directory with mode `0700` and stores no credential material in it." to improve clarity in docs/security/credential-storage.md.
73-73: Missing comma in conditional clause.Add a comma after "missing" for correct grammar.
-If a required credential is missing the deploy aborts before any remote work begins. +If a required credential is missing, the deploy aborts before any remote work begins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/security/credential-storage.md` at line 73, Update the sentence "If a required credential is missing the deploy aborts before any remote work begins." by inserting a comma after the word "missing" so it reads "If a required credential is missing, the deploy aborts before any remote work begins." This change targets the sentence fragment shown in the docs content to correct the conditional clause punctuation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/security/credential-storage.md`:
- Line 48: Break the single line into two lines with one sentence per line and
change the passive phrasing to active voice: rewrite the text as two sentences
such as "The CLI lists the provider names for which the gateway holds
credentials." and "The CLI does not display the credential values; this is a
deliberate property of OpenShell." Ensure each sentence sits on its own line in
docs/security/credential-storage.md.
- Around line 50-51: Reword the passive sentence to active voice: replace "That
directory is created with mode `0700` and contains no credential material." with
an active construction such as "NemoClaw creates that directory with mode `0700`
and stores no credential material in it." to improve clarity in
docs/security/credential-storage.md.
- Line 73: Update the sentence "If a required credential is missing the deploy
aborts before any remote work begins." by inserting a comma after the word
"missing" so it reads "If a required credential is missing, the deploy aborts
before any remote work begins." This change targets the sentence fragment shown
in the docs content to correct the conditional clause punctuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1c986e35-d420-4c65-bcbe-108b23fd6bfe
📒 Files selected for processing (2)
docs/security/credential-storage.mdsrc/lib/credentials.ts
After a multi-perspective regression review, fix the following: - Cap legacy credentials.json size at 1 MiB before reading, so an attacker with write access to ~/.nemoclaw/ cannot OOM the next onboard with a planted multi-GB file. Largest realistic credential set is well under 1 KiB; cap leaves comfortable headroom. - stageLegacyCredentialsToEnv now lstat's the legacy path (not statSync) and refuses anything that is not a regular file. A planted symlink at ~/.nemoclaw/credentials.json no longer redirects the JSON read at an unrelated file; the migration just bails. Pairs with the existing symlink-safe behavior in secureUnlink/removeLegacyCredentialsFile. - nemoclaw credentials reset now detects when the user passed a credential env-var name (UPPER_SNAKE_CASE shape) instead of a provider name. The CLI used to accept env-var names; the gateway now expects provider names. Print a one-paragraph hint pointing at `nemoclaw credentials list` so existing scripts surface their breakage instead of silently failing on "provider not found". - Export KNOWN_CREDENTIAL_ENV_KEYS so test/credentials.test.ts can import it instead of hand-maintaining a parallel list. Adding a new credential env key now only needs the one source-of-truth update. - Document on saveCredential that it mutates process.env directly (not via vi.stubEnv), so test files mutating credentials in-place must explicitly clean up. Regression tests: - Oversized legacy file → migration refuses, file is left for the user to inspect, no env var leaked. - Symlink at the legacy path → migration refuses, the pointee is not read or touched, no env var leaked. - Interrupted onboard → process A stages then "crashes" with file still present; process B starts fresh, re-stages idempotently, and succeeds with a clean removal. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/credentials.test.ts (1)
181-200:⚠️ Potential issue | 🟡 MinorPreserve inherited env vars instead of asserting
undefined.At Line 199 and Line 200, these assertions are still brittle on runners that already export
NODE_OPTIONSorOPENSHELL_GATEWAY. Capture their original values alongsidePATHand assert they remain unchanged after staging.Suggested fix
const originalPath = process.env.PATH; + const originalNodeOptions = process.env.NODE_OPTIONS; + const originalOpenShellGateway = process.env.OPENSHELL_GATEWAY; fs.writeFileSync( legacyFile, JSON.stringify({ PATH: "/attacker/bin:/usr/bin", @@ expect(staged).toEqual(["NVIDIA_API_KEY"]); expect(process.env.NVIDIA_API_KEY).toBe("nvapi-legitimate"); expect(process.env.PATH).toBe(originalPath); - expect(process.env.NODE_OPTIONS).toBeUndefined(); - expect(process.env.OPENSHELL_GATEWAY).toBeUndefined(); + expect(process.env.NODE_OPTIONS).toBe(originalNodeOptions); + expect(process.env.OPENSHELL_GATEWAY).toBe(originalOpenShellGateway);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credentials.test.ts` around lines 181 - 200, The test in test/credentials.test.ts asserts NODE_OPTIONS and OPENSHELL_GATEWAY are undefined which is brittle; update the test that calls importCredentialsModule(home) and stageLegacyCredentialsToEnv() to save original values for NODE_OPTIONS and OPENSHELL_GATEWAY (like originalPath is saved), then assert that process.env.NODE_OPTIONS and process.env.OPENSHELL_GATEWAY remain equal to their original saved values (not strictly undefined) after staging; keep the existing assertions for NVIDIA_API_KEY and PATH but replace the two undefined checks with equality checks against the captured originals.
🤖 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/credentials.ts`:
- Around line 237-260: In stageLegacyCredentialsToEnv() the code race-reads
legacyFile after lstatSync/size checks; instead open the file by descriptor
(fs.openSync) immediately after getCredsFile(), use fs.fstatSync(fd) to verify
isFile() and size against LEGACY_CREDS_FILE_MAX_BYTES, then read the file via
the descriptor (fs.readFileSync(fd, "utf-8") or equivalent) and finally close
the fd; keep getCredsFile() unchanged but replace the separate lstatSync +
readFileSync sequence with the pinned fd + fstatSync + read + close flow to
prevent symlink/file-swap races.
---
Duplicate comments:
In `@test/credentials.test.ts`:
- Around line 181-200: The test in test/credentials.test.ts asserts NODE_OPTIONS
and OPENSHELL_GATEWAY are undefined which is brittle; update the test that calls
importCredentialsModule(home) and stageLegacyCredentialsToEnv() to save original
values for NODE_OPTIONS and OPENSHELL_GATEWAY (like originalPath is saved), then
assert that process.env.NODE_OPTIONS and process.env.OPENSHELL_GATEWAY remain
equal to their original saved values (not strictly undefined) after staging;
keep the existing assertions for NVIDIA_API_KEY and PATH but replace the two
undefined checks with equality checks against the captured originals.
🪄 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: 0788850f-778f-44f3-86a1-a4a6f320927d
📒 Files selected for processing (3)
src/lib/credentials.tssrc/nemoclaw.tstest/credentials.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nemoclaw.ts
- shfmt rewrites the inline `command -v ... || { fail; exit 1; }`
one-liners in test/e2e/test-credential-migration.sh into the
multi-line form the rest of the e2e suite uses. The local pre-push
`Files were modified by following hooks` step was failing CI with no
underlying defect; this is purely a formatting alignment.
- Catch up the auto-generated user-configure-security skill so it
matches the keychain-fallback rewrite in
docs/security/credential-storage.md. Skill regeneration ran during
the previous prek invocation but the result was not committed.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
… gate Address four CodeRabbit findings: - stageLegacyCredentialsToEnv() previously did `lstatSync(path)` then `readFileSync(path, ...)` — two separate path-lookups an attacker could swap between to bypass both the symlink-refusal and the size cap. Pin the file by descriptor instead: `openSync(O_RDONLY | O_NOFOLLOW)` once, then fstat the fd for type/size checks and read from the fd. The open() itself fails on a symlinked final component, so the size and type checks now operate on the same inode the read consumes. - The post-onboard cleanup gate dropped from `stagedLegacyKeys.length > 0` to "every staged key was actually registered with the gateway in this run". Staging happens at onboard start, before provider selection and the messaging toggle, so a user picking a local provider or disabling a preselected messaging channel previously caused the legacy file to be unlinked even though those credentials never made it to the gateway. Track successful upserts via the existing upsertProvider / upsertMessagingProviders wrappers, reset the set at every onboard start, and only call removeLegacyCredentialsFile() when the staged set is fully covered by the registered set. When it isn't, keep the file and emit a one-liner naming the unregistered keys so the user knows what to do. - The CLI usage strings still referred to <KEY> while the docs and implementation now use <PROVIDER>. Align everything to <PROVIDER>: `nemoclaw credentials reset <PROVIDER> [--yes]` in the help, error, and post-onboard "what's next" footer. - The "ignores keys outside the credential allowlist" test asserted `process.env.NODE_OPTIONS === undefined`, which is brittle on runners that already export it. Capture the originals alongside PATH and assert preservation rather than absence. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…ned' into fix/credentials-gateway-only-signed
…e-match The upsertProvider wrapper compared the value openshell actually received to the staged legacy value via `env[credentialEnv] ?? process.env[credentialEnv]`. The PR-#2306 eslint rule `nemoclaw/no-direct-credential-env` flags the `process.env[credentialEnv]` branch because `credentialEnv` is a credential-shaped variable name; the rule's whole point is to funnel provider credential reads through getCredential() / resolveProviderCredential(). Switch the env-fallback branch to `getCredential(credentialEnv)` — same resolution order openshell follows when no env block value is provided, and the staging contract ensures process.env already mirrors the value under getCredential's view, so the comparison against stagedValue remains correct. No behavior change. `test/no-direct-credential-env.test.ts > onboard.ts has zero violations` now passes; targeted suites (credentials, no-direct-credential-env, canonical-credential-resolution, validate-e2e-coverage) all green at 41 tests. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…ay-only-signed # Conflicts: # .github/workflows/nightly-e2e.yaml
…ay-only-signed # Conflicts: # .github/workflows/nightly-e2e.yaml
…ay-only-signed # Conflicts: # .github/workflows/nightly-e2e.yaml # src/lib/onboard.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25081317655
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25081740765
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25081740765
|
cv
approved these changes
Apr 28, 2026
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25081317655
|
This was referenced Apr 29, 2026
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
…VIDIA#2554) ## Summary Make the OpenShell gateway the single system of record for provider credentials. NemoClaw no longer writes credentials to host disk; values are staged in process memory only long enough to register with the gateway via `openshell provider create/update`. ## Changes - `src/lib/credentials.ts` rewritten: `saveCredential` stages to `process.env` only; `getCredential` reads from env only; new `migrateLegacyCredentialsFile()` hydrates env from any pre-existing `~/.nemoclaw/credentials.json` and securely zero-fills + unlinks the file. - Migration runs at the start of `nemoclaw onboard` and inside `hydrateCredentialEnv`, so rebuild preflight and other credential-touching paths benefit from the one-time recovery without re-prompting. - `nemoclaw credentials list` queries `openshell provider list --names`; `nemoclaw credentials reset <NAME>` calls `openshell provider delete`. - `ensureGithubToken` no longer persists PATs. It tries `gh auth token` first (system keychain) and otherwise prompts for a session-only token, telling the user to run `gh auth login` for persistence. - `nemoclaw deploy` continues to read credentials from the host environment; missing values still fail clearly. Documented as the intended contract. - E2E shell scripts updated to `export NVIDIA_API_KEY=…` instead of writing through `saveCredential`, since the credential-staging side-effect no longer outlives the inline node process. - Docs (`docs/security/credential-storage.md`, `docs/reference/{commands,architecture,troubleshooting}.md`, `docs/get-started/quickstart.md`) rewritten to describe the gateway-only model and the legacy-file migration. Auto-generated user skills regenerated via `scripts/docs-to-skills.py`. ## Type of Change - [x] Code change with doc updates ## Verification - [x] `npm test` — credential-touched suites green (`test/credentials.test.ts`, `test/rebuild-credential-hydration.test.ts`, `test/rebuild-credential-preflight.test.ts`, `test/onboard.test.ts`). - [x] `npm run typecheck:cli` clean - [x] Tests added: legacy-migration scenarios (success, env precedence, no-file, corrupt input), no-host-file-written assertion, openshell upsert call shape verification - [x] No secrets, API keys, or credentials committed - [x] Docs updated for the user-facing behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Credentials are registered in the OpenShell gateway (never persisted to host disk). CLI stages creds into the environment for single-run operations; `credentials list` and `credentials reset <PROVIDER>` operate on gateway-registered providers. Onboard auto-migrates legacy local credential files into env staging, securely removes legacy files when migration completes, and refuses bridge-only names for resets. * **Documentation** * Quickstart, reference, troubleshooting, and security docs updated for gateway-centric credential handling, env-first behavior, deploy-time requirements, GitHub CLI/token guidance, and migration guidance. * **Tests / Chores** * Expanded unit tests, new E2E credential-migration test and nightly job, updated repair/resume scripts and migration-focused test suites. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
12 tasks
ericksoa
added a commit
that referenced
this pull request
May 7, 2026
…3119) ## Summary Securely remove a stale `~/.nemoclaw/credentials.json` left on host disk after upgrading from pre-gateway versions when the file has nothing to migrate (empty `{}`, only unknown keys, or only blank values). Closes the security-hygiene gap where cleartext API keys could linger indefinitely after the gateway-only migration in #2554. ## Related Issue Closes #3105 ## Changes - Add `removeLegacyCredentialsFileIfEmpty()` in `src/lib/credentials.ts` that re-inspects the legacy file under the same symlink-rejection / O_NOFOLLOW / size-cap guards used by `stageLegacyCredentialsToEnv()` and `secureUnlink`s it iff zero allowlisted keys carry a non-empty string value. - Add a new `src/lib/host-artifact-cleanup.ts` module exposing `cleanupStaleHostFiles()` — a tiny runner over a `STALE_FILES` const list. Today it has one entry (the legacy credentials file); future stale-file leftovers can be added by appending another entry without touching `onboard.ts`. Cleaner-level errors are isolated so a single failure can't abort onboard completion. - Wire `cleanupStaleHostFiles()` into the onboard success path immediately after the existing migrate/warn branches, so a fresh post-upgrade onboard sweeps any leftover artifacts in one uniform pass. - Tests cover the empty-file regression, unknown-keys-only, blank-value-only, real-credential preservation, missing file, symlink refusal, corrupt JSON, and zero-fill-before-unlink for defence-in-depth, plus end-to-end coverage of the runner. ## Type of Change - [x] 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 - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] 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](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Enhanced post-upgrade cleanup of legacy host artifacts with stricter safety checks * Credential files are now removed only when verified empty/unusable * Cleanup runs during onboarding finalization to sweep stale host files * **Tests** * Added comprehensive tests covering cleanup behavior, safety, idempotence, symlink protection, and recovery scenarios <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
wscurran
added
area: cli
This was referenced Jul 3, 2026
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
3 participants
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
Make the OpenShell gateway the single system of record for provider credentials. NemoClaw no longer writes credentials to host disk; values are staged in process memory only long enough to register with the gateway via
openshell provider create/update.Changes
src/lib/credentials.tsrewritten:saveCredentialstages toprocess.envonly;getCredentialreads from env only; newmigrateLegacyCredentialsFile()hydrates env from any pre-existing~/.nemoclaw/credentials.jsonand securely zero-fills + unlinks the file.nemoclaw onboardand insidehydrateCredentialEnv, so rebuild preflight and other credential-touching paths benefit from the one-time recovery without re-prompting.nemoclaw credentials listqueriesopenshell provider list --names;nemoclaw credentials reset <NAME>callsopenshell provider delete.ensureGithubTokenno longer persists PATs. It triesgh auth tokenfirst (system keychain) and otherwise prompts for a session-only token, telling the user to rungh auth loginfor persistence.nemoclaw deploycontinues to read credentials from the host environment; missing values still fail clearly. Documented as the intended contract.export NVIDIA_API_KEY=…instead of writing throughsaveCredential, since the credential-staging side-effect no longer outlives the inline node process.docs/security/credential-storage.md,docs/reference/{commands,architecture,troubleshooting}.md,docs/get-started/quickstart.md) rewritten to describe the gateway-only model and the legacy-file migration. Auto-generated user skills regenerated viascripts/docs-to-skills.py.Type of Change
Verification
npm test— credential-touched suites green (test/credentials.test.ts,test/rebuild-credential-hydration.test.ts,test/rebuild-credential-preflight.test.ts,test/onboard.test.ts).npm run typecheck:clicleanSummary by CodeRabbit
New Features
credentials listandcredentials reset <PROVIDER>operate on gateway-registered providers. Onboard auto-migrates legacy local credential files into env staging, securely removes legacy files when migration completes, and refuses bridge-only names for resets.Documentation
Tests / Chores