feat(onboard): add Tavily web search providers - #6165
Merged
Conversation
Revive Tavily onboarding from #2105 for the current OpenClaw and Hermes architectures. Co-authored-by: Lakshya Agarwal <lakshya.agarwal@tavily.com> Signed-off-by: Lakshya Agarwal <lakshya.agarwal@tavily.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
added
integration: openclaw
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:
📝 WalkthroughWalkthroughThis PR makes web-search handling provider-aware for Brave and Tavily across build-time config, onboarding, sandbox state, persistence, policy reconciliation, and documentation. It also adds Tavily-specific policy/profile artifacts and expands validation coverage. ChangesWeb Search Provider Generalization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WebSearchFlow
participant CurlProbe
participant MessagingPrep
User->>WebSearchFlow: choose brave or tavily
WebSearchFlow->>CurlProbe: validate provider API key
CurlProbe-->>WebSearchFlow: validation result
WebSearchFlow->>MessagingPrep: prepare sandbox messaging
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6165.docs.buildwithfern.com/nemoclaw |
Contributor
PR Review Advisor (Nemotron Ultra) — 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.
|
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Findings index
Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 1 in-scope improvement
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 3
🧹 Nitpick comments (8)
scripts/generate-openclaw-config.mts (1)
76-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared web-search provider contract here.
scripts/generate-openclaw-config.mtscan importsrc/lib/inference/web-search.tsundernode --experimental-strip-types, so thebrave/tavilyliterals and localWebSearchProvideralias don’t need to be redefined in this script; keeping them in one place avoids drift.🤖 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 `@scripts/generate-openclaw-config.mts` around lines 76 - 80, The web-search provider contract is being duplicated in the config generation script, which can drift from the shared source of truth. Update generate-openclaw-config.mts to import and reuse the provider definitions from src/lib/inference/web-search.ts instead of redefining WEB_SEARCH_PROVIDERS and WebSearchProvider locally, and make sure the script continues to reference the shared provider symbols when building the config.test/sandbox-provisioning-tavily.test.ts (1)
12-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
dockerRunCommandBetweenhelper.This function is identical to the one in
test/sandbox-provisioning.test.ts(lines 27-57 per provided context). Consider extracting it to a shared test helper module to avoid drift between the two copies.🤖 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/sandbox-provisioning-tavily.test.ts` around lines 12 - 36, The dockerRunCommandBetween helper is duplicated and should be shared to avoid divergence. Move the logic currently in dockerRunCommandBetween into a common test utility module and update both sandbox provisioning tests to import and use that shared helper, keeping the existing behavior and error messages intact.src/lib/onboard/web-search-flow.ts (1)
252-262: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
stageValidatedCredentialalways writes to the realprocess.env, defeating test isolation.
process.env[envKey] = apiKeyat Line 261 runs unconditionally, even when a test injects a customenvobject and a mockedsaveCredential. This is whyweb-search-flow.test.tsneeds manualdelete process.env.TAVILY_API_KEYcleanup in multiple tests — without it, credentials leak into the real environment for the remainder of the test run.♻️ Proposed fix: only touch the real env when no override is injected
function stageValidatedCredential(provider: WebSearchProvider, apiKey: string): void { const envKey = webSearchEnvFor(provider); persistCredential(envKey, apiKey); env[envKey] = apiKey; - process.env[envKey] = apiKey; + if (env !== process.env) process.env[envKey] = apiKey; }🤖 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/web-search-flow.ts` around lines 252 - 262, stageValidatedCredential currently mutates the real process.env on every call, which breaks test isolation when a custom env override is injected. Update stageValidatedCredential to continue persisting and updating the local env object, but only assign to process.env[envKey] when the function is running without an injected override (or otherwise detect that env is the real process.env). Use the existing symbols stageValidatedCredential, configuredCredential, env, and process.env to keep the fix scoped and avoid credential leakage across tests.src/lib/state/openclaw-config-merge.ts (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire
managedWebSearchConfigPathsintomergeOpenClawTools, or remove the unused contract.managedWebSearchConfigPathsis declared but never read, whilemergeOpenClawToolsstill hardcodestools.web.search; keeping both invites drift between the documented ownership contract and the actual merge behavior.🤖 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/state/openclaw-config-merge.ts` around lines 24 - 27, `managedWebSearchConfigPaths` is declared in the openclaw config merge contract but never used, while `mergeOpenClawTools` still hardcodes the web search path. Update `mergeOpenClawTools` in `openclaw-config-merge` to consume `managedWebSearchConfigPaths` when deciding which config paths are owned by a fresh web-search selection, or remove `managedWebSearchConfigPaths` from the contract if it is not intended to drive merge behavior. Keep the behavior and the documented ownership fields in sync with `managedWebSearchPluginEntries` and `mergeOpenClawTools`.src/lib/onboard/web-search-verify.ts (1)
255-265: 🚀 Performance & Scalability | 🔵 TrivialTavily 401/403 gets no remediation hint, unlike Brave.
The
provider === "brave"guard means Tavily auth failures (401/403) only get the generic "returned HTTP {status}" warning, without the recreate-sandbox guidance Brave gets for the equivalent "stale profile type" failure mode. If Tavily can hit the same legacy-profile-type failure class, consider extending the hint.🤖 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/web-search-verify.ts` around lines 255 - 265, The 401/403 remediation hint in web-search verification is only applied for the brave provider, so Tavily failures fall back to the generic warning. Update the conditional in onboard/web-search-verify.ts around the provider/status check to include Tavily where the same legacy-profile-type failure can occur, and keep the existing recreate-sandbox guidance in the shared warn path so both providers get the same hint.src/lib/onboard/messaging-prep.ts (1)
81-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
src/lib/onboard/messaging-prep.ts:50,90-100Remove the deprecatedmissingBraveApiKeyalias and switch the remaining tests tomissingWebSearchCredentialEnv; it has no runtime readers, and there’s no retirement link or exit criteria for keeping it.🤖 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/messaging-prep.ts` around lines 81 - 90, Remove the deprecated missingBraveApiKey alias from messaging-prep logic and keep using missingWebSearchCredentialEnv as the single source of truth. Update any related branching in the onboarding flow and adjust the tests/fixtures that still assert the old alias so they reference the remaining symbol instead. Ensure the cleanup is applied around braveProviderProfile.shouldEnableWebSearch and webSearch.webSearchEnvFor usage so there are no dead reads left.src/lib/onboard/machine/handlers/sandbox.ts (2)
173-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInject
envinstead of readingprocess.envdirectly.
resolveRequestedWebSearchConfigreadsprocess.env[WEB_SEARCH_PROVIDER_ENV]directly, unlike every other capability inSandboxStateFlow, which goes throughthis.deps.*, and unlikeweb-search-flow.ts's equivalent helpers, which resolveenv = deps.env ?? process.envand thread it through. This couples sandbox state resolution to the global process object and is inconsistent with the DI pattern used elsewhere in this file.♻️ Suggested refactor
-function resolveRequestedWebSearchConfig<WebSearchConfig>( - current: WebSearchConfig | null, -): WebSearchConfig | null { - const explicit = parseExplicitWebSearchProvider(process.env[WEB_SEARCH_PROVIDER_ENV]); +function resolveRequestedWebSearchConfig<WebSearchConfig>( + current: WebSearchConfig | null, + env: NodeJS.ProcessEnv = process.env, +): WebSearchConfig | null { + const explicit = parseExplicitWebSearchProvider(env[WEB_SEARCH_PROVIDER_ENV]);🤖 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/machine/handlers/sandbox.ts` around lines 173 - 180, resolveRequestedWebSearchConfig currently reads process.env directly, which breaks the dependency-injection pattern used by SandboxStateFlow and the web-search helpers. Update the function to accept an env source (or use the existing deps.env pattern from the surrounding flow), then pass that env through to parseExplicitWebSearchProvider instead of accessing process.env[WEB_SEARCH_PROVIDER_ENV] inline. Keep the change localized to resolveRequestedWebSearchConfig and its call sites so the sandbox state resolution stays consistent with the rest of the file.
506-518: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNote text can be misleading when no other Hermes tools remain.
When
hermesToolGatewayswas originally just["nous-web"], the filtered result is empty, but the note still says "keeping the other selected Nous tools," which is inaccurate in that case.💬 Suggested fix
if ( this.options.hermesToolGateways.includes("nous-web") && !hermesToolGateways.includes("nous-web") ) { - this.deps.note( - " Tavily Search replaces Hermes managed Web search/extract; keeping the other selected Nous tools.", - ); + this.deps.note( + hermesToolGateways.length > 0 + ? " Tavily Search replaces Hermes managed Web search/extract; keeping the other selected Nous tools." + : " Tavily Search replaces Hermes managed Web search/extract.", + ); }🤖 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/machine/handlers/sandbox.ts` around lines 506 - 518, The note in sandbox.ts is too broad because when effectiveHermesToolGatewaysForWebSearch() filters out the only selected Hermes gateway ("nous-web"), there are no remaining Nous tools to keep. Update the conditional around this.deps.note so it distinguishes between “some Hermes tools remain” and “none remain,” and change the message accordingly using hermesToolGateways, this.options.hermesToolGateways, and the existing note call to avoid misleading text.
🤖 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-nemohermes.mdx`:
- Around line 1969-1971: The environment-variable table in the Hermes-only
reference still describes OpenClaw-specific Brave/Tavily behavior, which
conflicts with the Hermes-only prose. Update the rows for
NEMOCLAW_WEB_SEARCH_PROVIDER and BRAVE_API_KEY in the command reference so they
only describe Hermes-supported web search behavior, remove Brave-first/OpenClaw
precedence wording, and ensure the table matches the existing Hermes-only
guidance elsewhere in the page.
In `@nemoclaw-blueprint/policies/presets/tavily.yaml`:
- Around line 21-26: The Tavily allowlist comment in the binaries block is
inaccurate because it says both Python paths are exact, but the entry in the
policy uses a glob for the managed interpreter. Update the comment near the
binaries entries to reflect the real invariant in this preset, keeping the
wording aligned with the actual path patterns used in the Tavily egress boundary
and referring to the path entries themselves rather than implying both are
exact.
In `@src/lib/state/openclaw-config-merge.ts`:
- Around line 130-147: The early return in mergeOpenClawTools bypasses the
fresh-generator ownership rule for tools.web.search, so a missing currentTools
can restore stale backup web-search settings. Update mergeOpenClawTools to
always apply the search-preservation/omission logic after choosing the fallback
source, ensuring the merged result never reintroduces backup tools.web.search
when currentTools omits it. Use the mergeJsonObjects, cloneJson, and
isPlainJsonObject handling in mergeOpenClawTools to preserve unrelated web
settings while forcing search to follow the currentTools state.
---
Nitpick comments:
In `@scripts/generate-openclaw-config.mts`:
- Around line 76-80: The web-search provider contract is being duplicated in the
config generation script, which can drift from the shared source of truth.
Update generate-openclaw-config.mts to import and reuse the provider definitions
from src/lib/inference/web-search.ts instead of redefining WEB_SEARCH_PROVIDERS
and WebSearchProvider locally, and make sure the script continues to reference
the shared provider symbols when building the config.
In `@src/lib/onboard/machine/handlers/sandbox.ts`:
- Around line 173-180: resolveRequestedWebSearchConfig currently reads
process.env directly, which breaks the dependency-injection pattern used by
SandboxStateFlow and the web-search helpers. Update the function to accept an
env source (or use the existing deps.env pattern from the surrounding flow),
then pass that env through to parseExplicitWebSearchProvider instead of
accessing process.env[WEB_SEARCH_PROVIDER_ENV] inline. Keep the change localized
to resolveRequestedWebSearchConfig and its call sites so the sandbox state
resolution stays consistent with the rest of the file.
- Around line 506-518: The note in sandbox.ts is too broad because when
effectiveHermesToolGatewaysForWebSearch() filters out the only selected Hermes
gateway ("nous-web"), there are no remaining Nous tools to keep. Update the
conditional around this.deps.note so it distinguishes between “some Hermes tools
remain” and “none remain,” and change the message accordingly using
hermesToolGateways, this.options.hermesToolGateways, and the existing note call
to avoid misleading text.
In `@src/lib/onboard/messaging-prep.ts`:
- Around line 81-90: Remove the deprecated missingBraveApiKey alias from
messaging-prep logic and keep using missingWebSearchCredentialEnv as the single
source of truth. Update any related branching in the onboarding flow and adjust
the tests/fixtures that still assert the old alias so they reference the
remaining symbol instead. Ensure the cleanup is applied around
braveProviderProfile.shouldEnableWebSearch and webSearch.webSearchEnvFor usage
so there are no dead reads left.
In `@src/lib/onboard/web-search-flow.ts`:
- Around line 252-262: stageValidatedCredential currently mutates the real
process.env on every call, which breaks test isolation when a custom env
override is injected. Update stageValidatedCredential to continue persisting and
updating the local env object, but only assign to process.env[envKey] when the
function is running without an injected override (or otherwise detect that env
is the real process.env). Use the existing symbols stageValidatedCredential,
configuredCredential, env, and process.env to keep the fix scoped and avoid
credential leakage across tests.
In `@src/lib/onboard/web-search-verify.ts`:
- Around line 255-265: The 401/403 remediation hint in web-search verification
is only applied for the brave provider, so Tavily failures fall back to the
generic warning. Update the conditional in onboard/web-search-verify.ts around
the provider/status check to include Tavily where the same legacy-profile-type
failure can occur, and keep the existing recreate-sandbox guidance in the shared
warn path so both providers get the same hint.
In `@src/lib/state/openclaw-config-merge.ts`:
- Around line 24-27: `managedWebSearchConfigPaths` is declared in the openclaw
config merge contract but never used, while `mergeOpenClawTools` still hardcodes
the web search path. Update `mergeOpenClawTools` in `openclaw-config-merge` to
consume `managedWebSearchConfigPaths` when deciding which config paths are owned
by a fresh web-search selection, or remove `managedWebSearchConfigPaths` from
the contract if it is not intended to drive merge behavior. Keep the behavior
and the documented ownership fields in sync with `managedWebSearchPluginEntries`
and `mergeOpenClawTools`.
In `@test/sandbox-provisioning-tavily.test.ts`:
- Around line 12-36: The dockerRunCommandBetween helper is duplicated and should
be shared to avoid divergence. Move the logic currently in
dockerRunCommandBetween into a common test utility module and update both
sandbox provisioning tests to import and use that shared helper, keeping the
existing behavior and error messages intact.
🪄 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: 09448491-9438-40d0-bb5e-4d2209c92b16
📒 Files selected for processing (82)
Dockerfileagents/hermes/Dockerfileagents/hermes/config/build-env.tsagents/hermes/config/hermes-config.tsagents/hermes/config/hermes-env.tsagents/hermes/config/managed-tool-gateway.tsagents/hermes/policy-permissive.yamlagents/hermes/seed-dashboard-config.pyagents/openclaw/policy-permissive.yamlci/platform-matrix.jsondocs/deployment/deploy-to-remote-gpu.mdxdocs/get-started/quickstart-hermes.mdxdocs/get-started/quickstart.mdxdocs/manage-sandboxes/runtime-controls.mdxdocs/network-policy/customize-network-policy.mdxdocs/network-policy/integration-policy-examples.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxdocs/reference/network-policies.mdxdocs/reference/platform-support.mdxdocs/reference/troubleshooting.mdxdocs/security/best-practices.mdxdocs/security/credential-storage.mdxnemoclaw-blueprint/policies/openclaw-sandbox-permissive.yamlnemoclaw-blueprint/policies/presets/tavily.yamlnemoclaw-blueprint/provider-profiles/tavily-hermes-v1.yamlscripts/generate-openclaw-config.mtsscripts/install.shsrc/lib/inference/web-search.test.tssrc/lib/inference/web-search.tssrc/lib/messaging/applier/build/messaging-build-applier.mtssrc/lib/onboard.tssrc/lib/onboard/brave-provider-profile.test.tssrc/lib/onboard/brave-provider-profile.tssrc/lib/onboard/dockerfile-patch.test.tssrc/lib/onboard/dockerfile-patch.tssrc/lib/onboard/extra-placeholder-keys.test.tssrc/lib/onboard/extra-placeholder-keys.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/core-flow-phases.tssrc/lib/onboard/machine/final-flow-phases.tssrc/lib/onboard/machine/flow-context.test.tssrc/lib/onboard/machine/flow-context.tssrc/lib/onboard/machine/handlers/policies.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/messaging-prep.test.tssrc/lib/onboard/messaging-prep.tssrc/lib/onboard/policy-presets.tssrc/lib/onboard/policy-resume-selection.test.tssrc/lib/onboard/policy-resume-selection.tssrc/lib/onboard/policy-selection.tssrc/lib/onboard/sandbox-messaging-preflight.test.tssrc/lib/onboard/sandbox-messaging-preflight.tssrc/lib/onboard/sandbox-provider-cleanup.tssrc/lib/onboard/summary.test.tssrc/lib/onboard/summary.tssrc/lib/onboard/web-search-flow.test.tssrc/lib/onboard/web-search-flow.tssrc/lib/onboard/web-search-support.test.tssrc/lib/onboard/web-search-support.tssrc/lib/onboard/web-search-verify.test.tssrc/lib/onboard/web-search-verify.tssrc/lib/policy/index.tssrc/lib/state/onboard-session.test.tssrc/lib/state/onboard-session.tssrc/lib/state/openclaw-config-merge.test.tssrc/lib/state/openclaw-config-merge.tstest/cli/destroy-detach-order.test.tstest/generate-hermes-config.test.tstest/generate-openclaw-config-web-search.test.tstest/generate-openclaw-config.test.tstest/hermes-gateway-wrapper.test.tstest/messaging-build-applier.test.tstest/onboard-brave-validation.test.tstest/onboard-policy-suggestions.test.tstest/sandbox-provider-cleanup.test.tstest/sandbox-provisioning-tavily.test.tstest/sandbox-provisioning.test.tstest/seed-hermes-dashboard-config.test.tstest/tavily-preset.test.tstest/validate-blueprint.test.ts
21 tasks
Tighten Tavily egress and preserve fresh OpenClaw search ownership. Isolate injected environment state and correct agent-specific documentation. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Apply exact search and extract rules to provider-derived policy. Document the shields-down exception and keep provisioning tests branch-free. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Automated-review triage for final head 892993a:
No code change is warranted for these findings. |
14 tasks
Merged
21 tasks
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
13 tasks
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/policy/index.ts (2)
1265-1274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant try/catch around a call that already fails soft.
runCapture(..., { ignoreError: true })never throws — per its implementation, every error branch (result.error, non-zerostatus, and the catch block) returns""whenignoreErroris set. Wrapping this specific call in an outer try/catch that also returnsnullis dead defensive code around a boundary with no realistic throw path.Based on learnings, avoid adding defensive error handling (try/catch wrappers, fallbacks) around internal helper logic when there is no realistic throwing path; only add it at boundaries where failures can realistically occur.
♻️ Simplify
function getGatewayPresets(sandboxName: string): string[] | null { - let rawPolicy = ""; - try { - rawPolicy = runCapture(buildPolicyGetFullCommand(sandboxName), { ignoreError: true }); - } catch { - return null; - } + const rawPolicy = runCapture(buildPolicyGetFullCommand(sandboxName), { ignoreError: true });🤖 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/policy/index.ts` around lines 1265 - 1274, The try/catch in getGatewayPresets is redundant because runCapture(..., { ignoreError: true }) already fails soft and returns an empty string instead of throwing. Remove the outer try/catch and keep the existing null return path based on parseCurrentPolicyOrEmpty(rawPolicy), using getGatewayPresets and runCapture as the key symbols to locate the logic.Source: Learnings
273-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate sandboxAgent lookup — extract a shared helper.
The
try { sandboxAgent = registry.getSandbox(sandboxName)?.agent ?? null; } catch { sandboxAgent = null; }block is duplicated verbatim at Line 273-280 (loadPresetForSandbox), Line 379-384 (listSetupPolicyPresets), and Line 1294-1299 (getGatewayPresets). Extracting agetSandboxAgentSafe(sandboxName)helper would avoid drift if the fallback logic ever needs to change.♻️ Proposed helper extraction
+function getSandboxAgentSafe(sandboxName: string): string | null { + try { + return registry.getSandbox(sandboxName)?.agent ?? null; + } catch { + return null; + } +} + function loadPresetForSandbox(sandboxName: string, presetName: string): string | null { - let sandboxAgent: string | null = null; - try { - sandboxAgent = registry.getSandbox(sandboxName)?.agent ?? null; - } catch { - sandboxAgent = null; - } + const sandboxAgent = getSandboxAgentSafe(sandboxName);Apply the same substitution at the other two call sites.
🤖 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/policy/index.ts` around lines 273 - 280, The sandbox agent lookup logic is duplicated in loadPresetForSandbox, listSetupPolicyPresets, and getGatewayPresets. Extract the try/catch fallback into a shared getSandboxAgentSafe(sandboxName) helper that returns registry.getSandbox(sandboxName)?.agent ?? null and swallows lookup errors, then replace all three inline blocks with calls to that helper to keep the fallback behavior consistent.
🤖 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.
Nitpick comments:
In `@src/lib/policy/index.ts`:
- Around line 1265-1274: The try/catch in getGatewayPresets is redundant because
runCapture(..., { ignoreError: true }) already fails soft and returns an empty
string instead of throwing. Remove the outer try/catch and keep the existing
null return path based on parseCurrentPolicyOrEmpty(rawPolicy), using
getGatewayPresets and runCapture as the key symbols to locate the logic.
- Around line 273-280: The sandbox agent lookup logic is duplicated in
loadPresetForSandbox, listSetupPolicyPresets, and getGatewayPresets. Extract the
try/catch fallback into a shared getSandboxAgentSafe(sandboxName) helper that
returns registry.getSandbox(sandboxName)?.agent ?? null and swallows lookup
errors, then replace all three inline blocks with calls to that helper to keep
the fallback behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 05956f9e-1943-45b0-bc37-f3c03687fa75
📒 Files selected for processing (23)
Dockerfileci/platform-matrix.jsondocs/get-started/quickstart-hermes.mdxdocs/get-started/quickstart-langchain-deepagents-code.mdxdocs/get-started/quickstart.mdxdocs/network-policy/customize-network-policy.mdxdocs/network-policy/integration-policy-examples.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxdocs/reference/network-policies.mdxdocs/reference/platform-support.mdxdocs/reference/troubleshooting.mdxdocs/security/best-practices.mdxscripts/install.shsrc/lib/onboard.tssrc/lib/onboard/dockerfile-patch.tssrc/lib/onboard/policy-resume-selection.tssrc/lib/onboard/policy-selection.tssrc/lib/onboard/web-search-flow.test.tssrc/lib/onboard/web-search-flow.tssrc/lib/policy/index.tstest/langchain-deepagents-code-image.test.tstest/onboard-policy-suggestions.test.ts
💤 Files with no reviewable changes (2)
- test/langchain-deepagents-code-image.test.ts
- test/onboard-policy-suggestions.test.ts
✅ Files skipped from review due to trivial changes (10)
- docs/reference/platform-support.mdx
- docs/get-started/quickstart-langchain-deepagents-code.mdx
- docs/network-policy/customize-network-policy.mdx
- docs/reference/network-policies.mdx
- ci/platform-matrix.json
- docs/reference/commands-nemohermes.mdx
- docs/network-policy/integration-policy-examples.mdx
- scripts/install.sh
- docs/reference/commands.mdx
- docs/reference/troubleshooting.mdx
🚧 Files skipped from review as they are similar to previous changes (10)
- src/lib/onboard/dockerfile-patch.ts
- docs/security/best-practices.mdx
- docs/get-started/quickstart-hermes.mdx
- docs/get-started/quickstart.mdx
- Dockerfile
- src/lib/onboard.ts
- src/lib/onboard/web-search-flow.test.ts
- src/lib/onboard/policy-resume-selection.ts
- src/lib/onboard/web-search-flow.ts
- src/lib/onboard/policy-selection.ts
7 tasks
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Adds first-class Tavily web-search onboarding for OpenClaw and Hermes, including provider-aware credentials, runtime configuration, network policy, rebuild/resume reconciliation, and live post-create verification. This revives the useful concepts from NVIDIA#2105 on the current architecture while preserving Brave compatibility and fail-closed behavior. ## Related Issue Advances NVIDIA#2718. Revives and supersedes NVIDIA#2105. The original Tavily contribution from @lakshyaag-tavily is preserved through co-author and sign-off trailers. ## Changes - Add shared `brave`, `tavily`, and `none` web-search selection with credential-store precedence, secure credential validation, provider-scoped resources, and legacy Brave migration. - Configure OpenClaw's bundled Tavily extension and Hermes' native Tavily backend, including managed-tool conflict suppression and provider-specific runtime verification. - Add least-privilege Tavily network policies, Hermes request-body credential rewriting, and coverage in both agent-specific and global permissive policies. - Reconcile provider changes across rebuild and resume without widening intentionally restricted policy state, and clean up stale provider config, credentials, and policies. - Document interactive and non-interactive setup, provider switching, policy behavior, troubleshooting, and credential handling. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: focused reviews covered credential handling, policy egress and body rewrites, provider switching, resume reconciliation, and runtime verification; no findings remain. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [x] 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) - [x] 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) Additional verification: - `npm test`: 946 files passed, 10,852 tests passed, 34 expected skips. - `make check`: passed, including coverage ratchets, source-shape and test-size budgets, gitleaks, ShellCheck, Hadolint, and plugin tests. - Post-rebase focused suite: 179 tests passed; `npm run typecheck:cli` passed. - `npm run docs`: 0 errors; the two existing Fern upgrade warnings remain. - Pinned OpenClaw and Hermes runtime contracts were inspected for the bundled extension and native backend behavior. --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added provider selection for web search during onboarding (Brave or Tavily), including provider-specific API key handling and sandbox recreation when changing providers. * Hermes now supports Tavily web search with correct backend routing and request credential rewriting; Tavily selection can replace the managed web gateway when applicable. * Added Tavily network policies/provider profiles with least-privilege access limited to `POST /search` and `POST /extract`. * **Bug Fixes** * Improved resume/reconciliation to correctly swap or remove stale web-search provider and related gateway selections. * Web-search verification now validates the active provider/backend and warns on misconfiguration without blocking completion. * **Documentation** * Updated onboarding quickstarts, references, and runtime controls to reflect the new provider variables, defaults, and rebuild/verification behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
This was referenced Jul 18, 2026
cv
pushed a commit
that referenced
this pull request
Jul 18, 2026
## Summary `nemoclaw rebuild --yes` failed preflight with `Brave Search credential is invalid. Brave Search requires BRAVE_API_KEY or a saved Brave Search credential in non-interactive mode.` for sandboxes whose web search works. `saveCredential` stages web-search keys to the process env only — the OpenShell gateway provider is the durable system of record — so a fresh rebuild process holds no host key, and the preflight demanded one it would never use: the OpenClaw recreate path already reuses the gateway-registered credential (`messaging-prep` `requiresExactOpenClawProviderBinding`). After this change the preflight accepts that same gateway credential-only provider binding, and rebuild succeeds without re-exporting `BRAVE_API_KEY`. ## Related Issue Fixes #7097 ## Changes - `src/lib/actions/sandbox/rebuild-target-runtime.ts`: before demanding a host key, `preflightRebuildWebSearchCredential` accepts a matching gateway credential-only provider binding (`<sandbox>-<provider>-search`, provider type, recorded credential key) read via `readGatewayProviderMetadata`, scoped to the sandbox's resolved gateway. The reuse is gated to the OpenClaw agent (`target.agentDefinition === null`) — the only recreate path that reuses the binding — and only when no host key is staged; a staged key and non-OpenClaw agents keep the existing validation path, and a missing/mismatched binding still fails closed. - `src/lib/actions/sandbox/rebuild-target-runtime.test.ts`: cover gateway-binding reuse, fail-closed on missing binding, staged-host-key validation, and the non-OpenClaw validation path. Scope note: issue #7097 also reports the balanced tier creating a global `brave` provider profile as a side effect. The `provider profile import` in NemoClaw has tolerated `already exists` since #6165, and decoupling the egress preset from the provider profile is a design decision (the profile drives the L7 proxy token rewrite), so it is not changed here; details on the issue. ## 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 - [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: no doc page documents a host-key requirement for rebuild; this removes an incorrect preflight failure so behavior matches the existing quickstart web-search docs. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: author review — the reuse check reads provider identity metadata only (`readGatewayProviderMetadata` never reads or exports credential values), requires the exact recorded name/type/credential-key binding with no config keys, is scoped to the sandbox's resolved gateway, and fails closed for mismatches, staged host keys, and non-OpenClaw agents. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: `npx vitest run --project cli src/lib/actions/sandbox/rebuild-target-runtime.test.ts` (7 passed); `npm run test:changed` (60 passed); `npx vitest run --project integration test/rebuild-credential-preflight.test.ts test/rebuild-stale-recovery.test.ts test/rebuild-shields-auto-unlock.test.ts` (13 passed); `npm run typecheck:cli` clean. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) --- Signed-off-by: Shawn Xie <shaxie@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Web-search credentials can now be reused during sandbox runtime recreation when an existing gateway credential matches the configured web-search provider. - **Bug Fixes** - Improved credential preflight logic: correctly revalidates when a staged host key is present, fails when neither a valid gateway credential nor host key is available, and preserves the expected behavior for non-OpenClaw agent configurations. - **Tests** - Added coverage for the new web-search credential reuse and failure scenarios during runtime preflight. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Shawn Xie <shaxie@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
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
4 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 first-class Tavily web-search onboarding for OpenClaw and Hermes, including provider-aware credentials, runtime configuration, network policy, rebuild/resume reconciliation, and live post-create verification. This revives the useful concepts from #2105 on the current architecture while preserving Brave compatibility and fail-closed behavior.
Related Issue
Advances #2718.
Revives and supersedes #2105. The original Tavily contribution from @lakshyaag-tavily is preserved through co-author and sign-off trailers.
Changes
brave,tavily, andnoneweb-search selection with credential-store precedence, secure credential validation, provider-scoped resources, and legacy Brave migration.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)Additional verification:
npm test: 946 files passed, 10,852 tests passed, 34 expected skips.make check: passed, including coverage ratchets, source-shape and test-size budgets, gitleaks, ShellCheck, Hadolint, and plugin tests.npm run typecheck:clipassed.npm run docs: 0 errors; the two existing Fern upgrade warnings remain.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
POST /searchandPOST /extract.