feat(credentials): add CLI subcommand to register provider credentials#5969
Merged
Conversation
Signed-off-by: Tinson Lai <tinsonl@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:
📝 WalkthroughWalkthroughAdds a Tavily provider profile, a Changescredentials:add and Tavily provider support
Sequence Diagram(s)sequenceDiagram
participant CredentialsAddCommand
participant runCredentialsAddAction
participant recordExtraProvider
participant registry
participant createSandbox
CredentialsAddCommand->>runCredentialsAddAction: provider, type, credential/config inputs
runCredentialsAddAction->>recordExtraProvider: record added provider
recordExtraProvider->>registry: addExtraProvider(name)
createSandbox->>registry: listExtraProviders()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 67%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
![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: 1
🧹 Nitpick comments (2)
test/cli/credentials-command.test.ts (2)
36-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid pinning these tests to Oclif’s exact validation text.
These assertions lock the suite to framework wording (
"Missing 1 required arg","Missing required flag type") instead of the command contract, so a harmless@oclif/corewording change would fail the tests even ifcredentials addstill rejects the inputs correctly. Prefer asserting the non-zero exit plus the presence of the missing arg/flag context. As per path instructions, "Review tests for behavioral confidence rather than implementation lock-in" and "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/credentials-command.test.ts` around lines 36 - 47, Update the credentials add CLI tests in credentials-command.test.ts so they verify behavior rather than Oclif’s exact wording: keep asserting the non-zero exit code from run(), but replace the string-matching on “Missing 1 required arg” and “Missing required flag type” with broader checks that the output indicates the provider argument and type flag are missing. Use the existing run() helper and the credentials add command cases as the public contract, not framework-specific validation text.Source: Path instructions
26-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new NemoClaw-specific failure branches.
The new suite exercises framework validation, but it never hits the custom
credentials addbranches for bridge-provider rejection or thealready existsreset hint insrc/commands/credentials/add.ts. Those are the new behaviors called out in the PR goals and are the ones most likely to regress independently of Oclif. As per path instructions, "Review tests for behavioral confidence rather than implementation lock-in" and "Require negative-path tests that prove the boundary rejects bypasses...", and the PR objectives explicitly call out bridge-name rejection plus thealready existsguidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/credentials-command.test.ts` around lines 26 - 62, The current tests cover oclif validation but miss the custom negative paths in credentials add. Add focused cases against the Add command that assert bridge-provider names are rejected and that the already-exists branch returns the reset hint guidance; use the existing run helper and keep assertions on the observable CLI output rather than implementation details.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/credentials/add.ts`:
- Around line 52-124: The CredentialsAddCommand.run method is doing
orchestration work instead of staying a thin parser/delegator. Move the gateway
recovery, OpenShell argv assembly, provider creation call, and stderr-to-UX
mapping out of run() into a dedicated action under src/lib/actions/**, and have
run() only parse flags/args and translate them into typed inputs for that
action. Keep the logic around provider, credentials, config, fromExisting, and
runOpenshellProviderCommand in the action layer, and preserve the same
success/error behavior there.
---
Nitpick comments:
In `@test/cli/credentials-command.test.ts`:
- Around line 36-47: Update the credentials add CLI tests in
credentials-command.test.ts so they verify behavior rather than Oclif’s exact
wording: keep asserting the non-zero exit code from run(), but replace the
string-matching on “Missing 1 required arg” and “Missing required flag type”
with broader checks that the output indicates the provider argument and type
flag are missing. Use the existing run() helper and the credentials add command
cases as the public contract, not framework-specific validation text.
- Around line 26-62: The current tests cover oclif validation but miss the
custom negative paths in credentials add. Add focused cases against the Add
command that assert bridge-provider names are rejected and that the
already-exists branch returns the reset hint guidance; use the existing run
helper and keep assertions on the observable CLI output rather than
implementation details.
🪄 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: 5216d902-3365-420f-aeb1-2344d88b9b65
📒 Files selected for processing (8)
nemoclaw-blueprint/provider-profiles/tavily.yamlsrc/commands/credentials.tssrc/commands/credentials/add.tssrc/lib/credentials/command-support.tssrc/lib/credentials/store.tstest/cli/credentials-command.test.tstest/credentials.test.tstest/validate-blueprint.test.ts
laitingsheng
added
NV QA
…derr Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/package-contract/cli/credentials-cli-command.test.ts (1)
316-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid pinning this package-contract test to the internal timeout value.
timeout: 30_000is an action detail, not part of the CLI contract, so harmless timeout tuning will break this test without changing behavior. Match the forwarded argv and user-visible outcome, or relaxoptsto a partial assertion.♻️ Suggested assertion change
- expect(calls).toEqual([ + expect(calls).toMatchObject([ { args: [ "provider", "create", "--name", "tavily-search", "--type", "tavily", "--credential", "TAVILY_API_KEY", ], - opts: { ignoreError: true, stdio: ["ignore", "pipe", "pipe"], timeout: 30_000 }, + opts: { ignoreError: true, stdio: ["ignore", "pipe", "pipe"] }, }, ]);As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/package-contract/cli/credentials-cli-command.test.ts` around lines 316 - 329, The package-contract test is asserting an internal action detail (`timeout: 30_000`) instead of the public CLI behavior, which makes it brittle. Update `credentials-cli-command.test.ts` to verify only the observable contract from the `calls` recorded by the CLI wrapper, keeping the forwarded argv and user-visible outcome while relaxing `opts` to ignore implementation-specific fields like timeout in the `expect(calls)` assertion.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/credentials-add.ts`:
- Around line 62-65: The validation branch in credentials-add.ts is echoing the
raw `credential` value back in the `fail(...)` message, which can leak secrets;
update the error handling in the `ENV_NAME_PATTERN` check to return a generic
invalid-credential message without interpolating user input, and add a
regression test around the `credentials-add` flow to verify a secret-like
`--credential` value is never reflected in stderr.
---
Nitpick comments:
In `@test/package-contract/cli/credentials-cli-command.test.ts`:
- Around line 316-329: The package-contract test is asserting an internal action
detail (`timeout: 30_000`) instead of the public CLI behavior, which makes it
brittle. Update `credentials-cli-command.test.ts` to verify only the observable
contract from the `calls` recorded by the CLI wrapper, keeping the forwarded
argv and user-visible outcome while relaxing `opts` to ignore
implementation-specific fields like timeout in the `expect(calls)` assertion.
🪄 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: f6728c3c-6fcd-4f73-8ec2-18fcdbbe33e0
📒 Files selected for processing (8)
src/commands/credentials/add.tssrc/lib/actions/credentials-add.tssrc/lib/cli/public-display-defaults.tssrc/lib/credentials/command-support.tssrc/lib/security/redact.tstest/cli/credentials-command.test.tstest/package-contract/cli/credentials-cli-command.test.tstest/secret-redaction.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/credentials/command-support.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cli/credentials-command.test.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/commands.mdx`:
- Around line 1984-1986: The commands reference for runCredentialsAddAction
needs to reflect the actual CLI constraint between --credential and
--from-existing. Update the documentation near the options table to state that
callers must provide either repeatable --credential <ENV_NAME> values or
--from-existing, but not both, and that invoking the command with neither is
invalid. Use the runCredentialsAddAction behavior as the source of truth so the
documented rules match the implemented validation.
🪄 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: 65a92292-7c0f-4735-9b55-97ebcf0ce5f5
📒 Files selected for processing (2)
docs/reference/commands.mdxsrc/lib/credentials/command-support.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/credentials/command-support.ts
…act tvly Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-5969.docs.buildwithfern.com/nemoclaw |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/security/secret-patterns.ts`:
- Around line 39-40: The Tavily secret pattern was added in the Node redaction
path, but the shell fallback still misses the new tvly- prefix. Update the
fallback redaction logic in debug.sh by adding tvly- to EXPECTED_SHELL_PREFIXES
(or the equivalent shell-side prefix list) so both paths redact the same Tavily
tokens. Keep the change aligned with the secret-patterns.ts Tavily rule to
ensure the Node and shell behavior stay equivalent.
🪄 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: 59899b34-e093-447c-866b-f1fa953e7315
📒 Files selected for processing (9)
docs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxnemoclaw-blueprint/provider-profiles/tavily.yamlsrc/lib/actions/credentials-add.tssrc/lib/security/secret-patterns.tstest/cli/credentials-command.test.tstest/package-contract/cli/credentials-cli-command.test.tstest/secret-redaction.test.tstest/validate-blueprint.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- test/cli/credentials-command.test.ts
- test/secret-redaction.test.ts
- nemoclaw-blueprint/provider-profiles/tavily.yaml
- docs/reference/commands.mdx
- test/package-contract/cli/credentials-cli-command.test.ts
- src/lib/actions/credentials-add.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard.ts (1)
2955-2965: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not attach the global extra-provider list to every sandbox.
registry.listExtraProviders()is global state, and this call now feeds all of it into sandbox creation. BecauseprepareSandboxCreatePlan()turns each entry into--provider, registering one provider makes it available to every sandbox created through this path, which is a least-privilege regression for secret-backed providers. Persist provider attachments per sandbox, or require explicit per-sandbox selection before passing them into create.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard.ts` around lines 2955 - 2965, The sandbox creation path is currently passing the global extra-provider registry into `sandboxCreatePlan.prepareSandboxCreatePlan()` via `registry.listExtraProviders()`, which makes every registered provider available to every sandbox. Remove this global attachment from the `prepareSandboxCreatePlan` call in `onboard.ts`, and instead source providers from per-sandbox state or an explicit sandbox-specific selection so only intended providers are converted into `--provider` entries.
🧹 Nitpick comments (1)
test/langchain-deepagents-code-image.test.ts (1)
1202-1202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a behavior assertion over the regex fingerprint literal.
This hard-codes
TOKEN_PREFIX_PATTERNS’ internal shape again instead of proving the public effect. Assert that atvly-...token is actually redacted/detected in the exercised boundary, rather than matching the exact regex source string. As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/langchain-deepagents-code-image.test.ts` at line 1202, The test is asserting the internal regex source for TOKEN_PREFIX_PATTERNS instead of the observable redaction behavior. Update the assertion in the langchain-deepagents-code-image test to verify that a real tvly-... token is detected/redacted through the public boundary exercised by the test, using the existing redaction flow rather than matching the literal pattern string.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/credentials/reset.ts`:
- Around line 69-70: The local cleanup in resetCredentials only happens when the
gateway delete succeeds with a zero status, so stale extraProviders can remain
when the provider was already removed elsewhere. Update the delete-result
handling in resetCredentials to also treat the gateway’s “not found/already
absent” response as a successful cleanup path, and call removeExtraProvider(key)
for that case as well; keep the existing success handling around result.status
and the provider key flow unchanged.
In `@src/lib/onboard/sandbox-create-plan.test.ts`:
- Around line 238-315: The current tests only cover prepareSandboxCreatePlan’s
internal provider rewrite and do not verify the public entrypoints that are
supposed to reach it. Add a test around the top-level onboard/rebuild flow that
exercises persisted extraProviders coming from registry state and asserts they
are carried through to the final create args, using the relevant entrypoint
functions rather than calling prepareSandboxCreatePlan directly. Also add
coverage that the old provider-attachment path is no longer reachable or has
been removed, so a wiring regression above this helper will fail.
In `@test/registry.test.ts`:
- Around line 523-535: The current `removeExtraProvider` test only validates the
in-memory result and misses the disk serialization behavior. Update the
`removeExtraProvider clears the entry and drops the field when empty` case in
`registry.test.ts` to assert the persisted registry data no longer contains
`extraProviders` after the last removal, using the `regFile` JSON readback or
the `serializeRegistryForDisk()` behavior to confirm the field is deleted rather
than left as an empty array.
---
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 2955-2965: The sandbox creation path is currently passing the
global extra-provider registry into
`sandboxCreatePlan.prepareSandboxCreatePlan()` via
`registry.listExtraProviders()`, which makes every registered provider available
to every sandbox. Remove this global attachment from the
`prepareSandboxCreatePlan` call in `onboard.ts`, and instead source providers
from per-sandbox state or an explicit sandbox-specific selection so only
intended providers are converted into `--provider` entries.
---
Nitpick comments:
In `@test/langchain-deepagents-code-image.test.ts`:
- Line 1202: The test is asserting the internal regex source for
TOKEN_PREFIX_PATTERNS instead of the observable redaction behavior. Update the
assertion in the langchain-deepagents-code-image test to verify that a real
tvly-... token is detected/redacted through the public boundary exercised by the
test, using the existing redaction flow rather than matching the literal pattern
string.
🪄 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: 40c639ee-35fb-40f3-b120-6fc3c91bde5b
📒 Files selected for processing (12)
src/commands/credentials/reset.tssrc/lib/actions/credentials-add.tssrc/lib/actions/global.tssrc/lib/onboard.tssrc/lib/onboard/sandbox-create-plan.test.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/security/secret-patterns.tssrc/lib/state/registry.tstest/deepagents-code-tui-startup-check.test.tstest/langchain-deepagents-code-image.test.tstest/package-contract/cli/credentials-cli-command.test.tstest/registry.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/security/secret-patterns.ts
- src/lib/actions/credentials-add.ts
- test/package-contract/cli/credentials-cli-command.test.ts
…rd.ts Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…eset stderr Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…is gone Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/credentials/reset.ts`:
- Around line 77-85: The reset flow in credentials reset.ts incorrectly treats
uppercase provider names as env vars via looksLikeEnvName, which can skip
cleanup for valid providers like TAVILY_SEARCH when the gateway says they are
already absent. Update the alreadyAbsent handling in the reset command so it
distinguishes real env-var keys from provider names using a more reliable check
than the current regex, and ensure forgetExtraProvider(key) still runs for
deleted providers. Keep the cleanup path and the user-facing logs in the reset
command consistent with the provider state before returning.
In `@test/registry.test.ts`:
- Around line 532-535: The registry “round-trip” test only inspects the saved
JSON and never verifies deserialization through the public API. Update the test
around registry.addExtraProvider and registry.listExtraProviders() so it reopens
or rehydrates the registry state from disk after writing, then assert the
provider is still present via listExtraProviders() instead of checking
raw.extraProviders directly.
- Around line 538-577: This test is asserting the helper directly instead of the
integration path that loads persisted providers from the registry. Update the
test to exercise the public create/rebuild boundary that wires registry state
into sandbox creation, using the real flow that calls prepareSandboxCreatePlan
rather than passing registry.listExtraProviders() in manually. Keep the
assertion on the observable provider args, but verify them through the caller
responsible for propagating registry providers into the plan.
🪄 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: 34497390-603b-441e-bd0c-af07508f8e4e
📒 Files selected for processing (16)
docs/get-started/quickstart-langchain-deepagents-code.mdxsrc/commands/credentials.test.tssrc/commands/credentials/reset.tssrc/lib/actions/credentials-add.tssrc/lib/actions/global.tssrc/lib/onboard.tssrc/lib/onboard/sandbox-create-plan.test.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/security/secret-patterns.tssrc/lib/state/registry.tstest/deepagents-code-tui-startup-check.test.tstest/e2e/e2e-cloud-experimental/checks/07-deepagents-code-headless-inference.shtest/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.shtest/langchain-deepagents-code-image.test.tstest/package-contract/cli/credentials-cli-command.test.tstest/registry.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- test/langchain-deepagents-code-image.test.ts
- test/deepagents-code-tui-startup-check.test.ts
- src/lib/onboard/sandbox-create-plan.test.ts
- src/lib/onboard/sandbox-create-plan.ts
- src/lib/actions/credentials-add.ts
- src/lib/state/registry.ts
- src/lib/onboard.ts
…vider grammar Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/credentials-add.ts (1)
106-110: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon’t echo the rejected
--credentialprefix.The KEY=VALUE branch still reflects user input before
=, which can leak secret-like values in logs. Use a fixed example instead.Proposed fix
if (credential.includes("=")) { return fail([ ` --credential expects an env variable name, not 'KEY=VALUE'.`, - ` Export the value first (e.g. \`export ${credential.split("=", 1)[0]}=...\`)`, - ` and re-run with \`--credential ${credential.split("=", 1)[0]}\`.`, + " Export the value first (e.g. `export TAVILY_API_KEY=...`)", + " and re-run with `--credential TAVILY_API_KEY`.", ]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/credentials-add.ts` around lines 106 - 110, The `credential.includes("=")` validation branch in `credentials-add.ts` is echoing back user-provided content from `--credential`, which can leak sensitive values. Update the `fail(...)` संदेश in this branch to avoid using `credential.split("=", 1)[0]` in any output and replace it with a fixed, non-user-derived example in both the export and re-run guidance. Keep the change localized to the `credential.includes("=")` check and its `fail` message.
🧹 Nitpick comments (1)
src/lib/actions/credentials-add.ts (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove bundled-profile filesystem probing out of the action.
This action now owns repo-file discovery via
fs.existsSync/ROOT. Keep this module orchestration-only and put bundled profile lookup/import support behind asrc/lib/credentials/**or adapter helper.As per path instructions, actions should orchestrate while adapters own host/process/network boundaries, and credential-command support belongs under
src/lib/credentials/**.Also applies to: 45-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/credentials-add.ts` around lines 4 - 5, The bundled-profile filesystem probing currently lives in the credentials-add action, which should stay orchestration-only. Move the ROOT/fs.existsSync-based repo/file discovery and bundled profile import support into a helper under src/lib/credentials/** or an adapter, and have the action call that helper instead. Keep the action focused on wiring command flow, and update the referenced imports/usages in credentials-add so host filesystem concerns are no longer handled there.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/actions/credentials-add.ts`:
- Around line 106-110: The `credential.includes("=")` validation branch in
`credentials-add.ts` is echoing back user-provided content from `--credential`,
which can leak sensitive values. Update the `fail(...)` संदेश in this branch to
avoid using `credential.split("=", 1)[0]` in any output and replace it with a
fixed, non-user-derived example in both the export and re-run guidance. Keep the
change localized to the `credential.includes("=")` check and its `fail` message.
---
Nitpick comments:
In `@src/lib/actions/credentials-add.ts`:
- Around line 4-5: The bundled-profile filesystem probing currently lives in the
credentials-add action, which should stay orchestration-only. Move the
ROOT/fs.existsSync-based repo/file discovery and bundled profile import support
into a helper under src/lib/credentials/** or an adapter, and have the action
call that helper instead. Keep the action focused on wiring command flow, and
update the referenced imports/usages in credentials-add so host filesystem
concerns are no longer handled there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 369615bf-dfe4-40e4-8bd6-eef9cf424501
📒 Files selected for processing (2)
src/lib/actions/credentials-add.tstest/package-contract/cli/credentials-cli-command.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/package-contract/cli/credentials-cli-command.test.ts
22 tasks
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28464821224
|
Contributor
Vitest E2E Target Results — ✅ All selected jobs passedRun: 28465069138
|
…vily binary allowlist Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28465065066
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
26 tasks
This was referenced Jul 1, 2026
apurvvkumaria
added a commit
that referenced
this pull request
Jul 1, 2026
<!-- markdownlint-disable MD041 --> ## Summary Restore the managed Deep Agents Code Python interpreter to the Tavily policy and provider-profile allowlists. PR #5969 removed Python while tightening the Tavily boundary, which left `policy-add tavily` successful but Python-originated Tavily requests blocked by OpenShell with HTTP 403. ## Changes - Add `/opt/venv/bin/python3*` to `nemoclaw-blueprint/policies/presets/tavily.yaml`. - Add the same managed interpreter path to `nemoclaw-blueprint/provider-profiles/tavily.yaml` so both enforcement layers agree. - Document why the interpreter is required for OpenShell process attribution. - Update the exact preset and provider-profile allowlist contracts. - Keep system Python paths excluded so the restored access remains limited to the managed Deep Agents environment. ## 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) ## Quality Gates <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: existing Deep Agents documentation already states that OpenShell attributes Tavily calls to the managed Python interpreter; this restores the documented behavior. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: pending maintainer review; the change restores only `/opt/venv/bin/python3*`, not system Python paths. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [ ] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes — the diff-scoped fallback passed formatting, schema, repository, and secret-scan hooks; its broad integration lane requires Linux utilities/semantics unavailable on this macOS host, so CI remains authoritative. - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run 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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Apurv Kumaria <36614+apurvvkumaria@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated Tavily-related runtime permissions to allow the managed Python interpreter, which should reduce permission-related failures when running Python-based tasks. * Kept existing allowed binaries unchanged while extending support for the additional Python runtime path. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <36614+apurvvkumaria@users.noreply.github.com> Co-authored-by: Apurv Kumaria <36614+apurvvkumaria@users.noreply.github.com>
3 tasks
ericksoa
pushed a commit
that referenced
this pull request
Jul 2, 2026
## Summary - Add the `v0.0.72` release-note section with links to the deeper docs pages for installer recovery, command diagnostics, inference, policy, and sandbox repair changes. - Document the custom preset `allowed_ips` guard for user-authored policy files. ## Related Issue None. ## Source summary - #6132 -> `docs/about/release-notes.mdx`: Summarizes installer and upgrade recovery before generic onboarding, with links to quickstart and lifecycle docs. - #6087 -> `docs/network-policy/customize-network-policy.mdx`: Documents that user-authored custom presets reject `allowed_ips` for ordinary endpoints; also summarized in release notes. - #5975 -> `docs/about/release-notes.mdx`: Summarizes safer curl-based inference probes that keep API keys out of process arguments. - #6044 -> `docs/about/release-notes.mdx`: Summarizes compact `channels status` configuration reporting. - #6096 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw EC2 metadata discovery disablement and links to security guidance. - #5980 and #5991 -> `docs/about/release-notes.mdx`: Summarizes `exec` multiline argument rejection and recovery guidance. - #6023 -> `docs/about/release-notes.mdx`: Summarizes registered-provider diagnostics for `inference set` failures. - #6074 -> `docs/about/release-notes.mdx`: Summarizes the refreshed NVIDIA Endpoints featured-model selection behavior. - #5969 -> `docs/about/release-notes.mdx`: Summarizes `credentials add` provider credential registration. - #6060 -> `docs/about/release-notes.mdx`: Summarizes mutable OpenClaw config permission restoration after `exec`. - #6134 -> `docs/about/release-notes.mdx`: Summarizes restored Tavily access for managed Python workflows. - #6089 -> `docs/about/release-notes.mdx`: Summarizes Hermes runtime version-scheme comparison during upgrade checks. - #6131 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw gateway watchdog recovery behavior. - #5976 and #5990 -> `docs/about/release-notes.mdx`: Summarizes prompt stdin EOF cancellation behavior during onboarding. - #5540 -> `docs/about/release-notes.mdx`: Summarizes clarified host-level and per-sandbox status command scope. - #5978 and #6018 -> `docs/about/release-notes.mdx`: Summarizes policy-denial log breadcrumbs in connect shells. ## Testing - `npm run docs:sync-agent-variants` - `npm run docs` - Commit hooks passed during `git commit`, including commitlint and gitleaks. - Pre-push hook passed during `git push`, including TypeScript CLI and package/tag version sync. ## Checklist - [x] Documentation updated. - [x] `npm run docs` completed with 0 errors and 1 existing Fern warning. - [x] No source code or generated build artifacts committed. Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.72 covering improved installer recovery, clearer CLI diagnostics, safer inference setup and provider switching, better credential handling, stronger policy boundaries, and more robust runtime repair behavior. * Updated network policy guidance to clarify when `allowed_ips` can be used, including a specific exception for the sandbox-to-host bridge endpoint. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
Adds
nemoclaw credentials add <PROVIDER> --type <TYPE>so users can register OpenShell provider credentials from NemoClaw without dropping to the OpenShell CLI. Ships a Tavily Search provider profile alongside so--type tavilyworks out of the box.Related Issue
Fixes #5965
Resolves #1720
Changes
credentials addsubcommand wrappingopenshell provider create. Accepts--type,--credential KEY[=VALUE](repeatable),--config K=V(repeatable), or--from-existing. Rejects messaging-bridge names and surfaces analready existshint that points atcredentials reset.nemoclaw-blueprint/provider-profiles/tavily.yaml(hostapi.tavily.com, bearer authorization,TAVILY_API_KEY).TAVILY_API_KEYadded toKNOWN_CREDENTIAL_ENV_KEYSso sanitization and rotation cover it.credentialsusage and the dispatch banner now show<list|add|reset>.Type of Change
Quality Gates
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
credentials addto register provider credentials/config (supports--from-existingand repeatable--credential/--config).TAVILY_API_KEYand Tavily-style tokens.credentials resetcleanup and ensured provider-delete stderr is redacted.$$nemoclaw credentials add <PROVIDER>(and nemohermes), plus Tavily quickstart guidance.credentials add/reset, Tavily profile validation, extra-provider persistence, and secret redaction.