feat(onboard): extend web search onboarding to Gemini and Tavily#1497
Open
13ernkastel wants to merge 5 commits intoNVIDIA:mainfrom
Open
feat(onboard): extend web search onboarding to Gemini and Tavily#149713ernkastel wants to merge 5 commits intoNVIDIA:mainfrom
13ernkastel wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
Signed-off-by: 13ernkastel <LennonCMJ@live.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:
📝 WalkthroughWalkthroughGeneralizes web-search onboarding from Brave-only to a provider model (Brave, Gemini, Tavily); adds provider registry/normalization and provider-aware onboarding, validation, and Docker config generation. Changes sandbox/Docker wiring to expose a writable live OpenClaw config at Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Onboard CLI
participant Validator as Credential Validator
participant Config as Config Builder
participant Docker as Dockerfile Patcher
rect rgba(200,150,255,0.5)
Note over User,Docker: Interactive provider-aware onboarding
User->>CLI: start onboarding
CLI->>CLI: normalize persisted webSearchConfig
alt request web search
CLI->>User: prompt/select provider
User->>CLI: select provider + (maybe apiKey)
CLI->>Validator: validate(provider, apiKey)
alt valid
Validator-->>CLI: valid
CLI->>Config: buildWebSearchConfigFragment(provider, apiKey)
Config-->>CLI: config fragment (provider, optional apiKey, fetchEnabled)
CLI->>Docker: patch Dockerfile with encoded config
Docker-->>CLI: patched Dockerfile
else invalid
Validator-->>CLI: invalid
CLI->>User: prompt recovery / back / skip
end
end
CLI-->>User: persist session with webSearchConfig
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/onboard.js (1)
4195-4220:⚠️ Potential issue | 🟠 MajorPreserve the saved “web search disabled” choice on resume.
normalizeWebSearchConfigValue()collapses a stored{ fetchEnabled: false }tonull, so this branch re-entersconfigureWebSearch()even after the user already made a decision. If they enable or switch providers here, Step 5 and Step 7 can still be skipped from the old session state, leavingcurrent.webSearchConfigout of sync with the sandbox image and applied presets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 4195 - 4220, normalizeWebSearchConfigValue() is collapsing a saved "{ fetchEnabled: false }" into null so the resume path incorrectly calls configureWebSearch() and may change the user’s disabled choice; fix by reading the raw persisted value (persistedWebSearchConfigRaw) before normalization and if that raw object explicitly has fetchEnabled === false, set webSearchConfig = { fetchEnabled: false } and call onboardSession.updateSession to restore it without invoking configureWebSearch() or ensureValidatedWebSearchCredential(); otherwise continue using the normalized path (persistedWebSearchConfig, getWebSearchProviderMetadata, ensureValidatedWebSearchCredential, configureWebSearch) as currently implemented.
🧹 Nitpick comments (3)
test/onboard.test.js (1)
253-259: Table-drive the web-search Dockerfile patch tests.These cases now exercise the same path with only provider/env literals changed. A small provider matrix would remove the duplication and make it easy to cover Tavily here too, so credential lookup stays aligned across all three providers.
Also applies to: 277-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 253 - 259, Replace the repeated provider-specific assertions with a table-driven loop: create an array/matrix of provider cases (e.g., [{provider: "brave", key: "brv-test-key"}, {provider: "tavily", key: "tav-test-key"}, ...]) and iterate with test.each or forEach to invoke the same setup, call the code that writes the Dockerfile, read it with fs.readFileSync(dockerfilePath, "utf8"), and compare against buildWebSearchDockerConfig(...) for each case; update the assertions currently using literal provider/env values to use the loop variables so the credential lookup is exercised uniformly across providers (affecting the block around buildWebSearchDockerConfig, dockerfilePath, and fs.readFileSync).src/lib/onboard-session.ts (1)
13-13: Derive credential env names fromweb-searchinstead of hard-coding them here.
TAVILY_API_KEYis another manual addition to this regex. Sincesrc/lib/web-search.tsalready owns the provider-to-credential mapping, build the redaction pattern from that source of truth so the next provider addition does not leak secrets into persisted failure text.Also applies to: 126-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-session.ts` at line 13, Replace the hard-coded credential env names/TAVILY_API_KEY in onboard-session.ts with a derived list from the web-search module instead of literals: import (or add and export) the provider->credential mapping or a helper that returns credential env names from src/lib/web-search.ts (you already import normalizeWebSearchConfig and WebSearchConfig there), generate the redaction RegExp from that list at module init, and use that single computed regex in both places where you currently hard-code names (the initial import/regex and the later redaction use). Ensure the web-search export you rely on is stable (or add a small accessor) so future provider additions automatically update the redaction pattern.src/lib/web-search.test.ts (1)
66-100: Add Tavily coverage for the emitted config shape.Brave and Gemini verify the fragment/docker JSON, but Tavily is only covered through warning text. A broken Tavily
pluginEntryorprovidermapping would still pass this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/web-search.test.ts` around lines 66 - 100, Add a unit test that asserts buildWebSearchDockerConfig produces the expected JSON for provider "tavily" (mirror the existing Gemini/Brave test): call buildWebSearchDockerConfig({ provider: "tavily", fetchEnabled: true }, "<key>"), base64-decode and JSON.parse the result, then assert the plugins.entries contains the correct tavily plugin entry and config (API key passed through) and that tools.web.search.provider === "tavily" and tools.web.fetch.enabled === true; this will catch broken pluginEntry/provider mappings related to the buildWebSearchDockerConfig and ensure getWebSearchExposureWarningLines coverage complements the config shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 1063-1126: ensureValidatedWebSearchCredential currently always
calls promptWebSearchApiKey and promptWebSearchRecovery, which blocks in
--non-interactive mode; update it to detect non-interactive mode (use the same
check used by promptValidationRecovery) and fail fast instead of prompting: if
non-interactive and no saved apiKey, log an error/validation message and return
null (do not call promptWebSearchApiKey); likewise after
validateWebSearchCredential if validation fails and non-interactive, emit the
same error messages and return null (do not call promptWebSearchRecovery).
Reference functions/variables: ensureValidatedWebSearchCredential,
promptWebSearchApiKey, promptWebSearchRecovery, validateWebSearchCredential, and
the non-interactive check used by promptValidationRecovery.
In `@docs/reference/commands.md`:
- Around line 91-100: The existing warning about secret exposure currently
mentions only Brave keys; update the docs so the warning states that whichever
provider key (BRAVE_API_KEY, GEMINI_API_KEY, or TAVILY_API_KEY) is
selected—either by precedence or explicitly via
NEMOCLAW_WEB_SEARCH_PROVIDER—will be copied into the sandbox OpenClaw config and
thus visible to agents when web_search (and web_fetch) are enabled; adjust the
wording near the provider list to be provider-agnostic and mention that the
selected provider's apiKey is what gets exposed.
In `@src/lib/web-search.ts`:
- Around line 77-83: The normalizeWebSearchConfig function currently uses
parseWebSearchProvider(value.provider) || "brave", which hides invalid/unknown
persisted providers by falling back to Brave; instead, change
normalizeWebSearchConfig so that if the provider field is absent (value.provider
=== undefined) it defaults to "brave", but if a provider is present and
parseWebSearchProvider(...) returns null treat the config as invalid (return
null) so typos/unsupported providers are surfaced; keep fetchEnabled: true and
use parseWebSearchProvider to validate when provider is present.
---
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 4195-4220: normalizeWebSearchConfigValue() is collapsing a saved
"{ fetchEnabled: false }" into null so the resume path incorrectly calls
configureWebSearch() and may change the user’s disabled choice; fix by reading
the raw persisted value (persistedWebSearchConfigRaw) before normalization and
if that raw object explicitly has fetchEnabled === false, set webSearchConfig =
{ fetchEnabled: false } and call onboardSession.updateSession to restore it
without invoking configureWebSearch() or ensureValidatedWebSearchCredential();
otherwise continue using the normalized path (persistedWebSearchConfig,
getWebSearchProviderMetadata, ensureValidatedWebSearchCredential,
configureWebSearch) as currently implemented.
---
Nitpick comments:
In `@src/lib/onboard-session.ts`:
- Line 13: Replace the hard-coded credential env names/TAVILY_API_KEY in
onboard-session.ts with a derived list from the web-search module instead of
literals: import (or add and export) the provider->credential mapping or a
helper that returns credential env names from src/lib/web-search.ts (you already
import normalizeWebSearchConfig and WebSearchConfig there), generate the
redaction RegExp from that list at module init, and use that single computed
regex in both places where you currently hard-code names (the initial
import/regex and the later redaction use). Ensure the web-search export you rely
on is stable (or add a small accessor) so future provider additions
automatically update the redaction pattern.
In `@src/lib/web-search.test.ts`:
- Around line 66-100: Add a unit test that asserts buildWebSearchDockerConfig
produces the expected JSON for provider "tavily" (mirror the existing
Gemini/Brave test): call buildWebSearchDockerConfig({ provider: "tavily",
fetchEnabled: true }, "<key>"), base64-decode and JSON.parse the result, then
assert the plugins.entries contains the correct tavily plugin entry and config
(API key passed through) and that tools.web.search.provider === "tavily" and
tools.web.fetch.enabled === true; this will catch broken pluginEntry/provider
mappings related to the buildWebSearchDockerConfig and ensure
getWebSearchExposureWarningLines coverage complements the config shape.
In `@test/onboard.test.js`:
- Around line 253-259: Replace the repeated provider-specific assertions with a
table-driven loop: create an array/matrix of provider cases (e.g., [{provider:
"brave", key: "brv-test-key"}, {provider: "tavily", key: "tav-test-key"}, ...])
and iterate with test.each or forEach to invoke the same setup, call the code
that writes the Dockerfile, read it with fs.readFileSync(dockerfilePath,
"utf8"), and compare against buildWebSearchDockerConfig(...) for each case;
update the assertions currently using literal provider/env values to use the
loop variables so the credential lookup is exercised uniformly across providers
(affecting the block around buildWebSearchDockerConfig, dockerfilePath, and
fs.readFileSync).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93b915b8-8a9d-4434-9363-fc04e0de64c3
📒 Files selected for processing (11)
Dockerfilebin/lib/onboard.jsdocs/reference/commands.mdnemoclaw-blueprint/policies/presets/gemini.yamlnemoclaw-blueprint/policies/presets/tavily.yamlsrc/lib/onboard-session.test.tssrc/lib/onboard-session.tssrc/lib/web-search.test.tssrc/lib/web-search.tstest/onboard.test.jstest/policies.test.js
Author
|
Follow-up update in
Targeted validation that passed locally:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/web-search.ts`:
- Line 31: The webSearch config fragment built by buildWebSearchConfigFragment
currently only spreads apiKey and omits the model; update that function so when
the provider/pluginEntry is "gemini" you include model:
DEFAULT_GEMINI_WEB_SEARCH_MODEL in the webSearch object (e.g., add ...(provider
=== "gemini" ? { model: DEFAULT_GEMINI_WEB_SEARCH_MODEL } : {})), keeping the
existing conditional apiKey spread; reference DEFAULT_GEMINI_WEB_SEARCH_MODEL
and buildWebSearchConfigFragment so the Google plugin's webSearch config
includes the Gemini model when applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd7b32e5-6975-4b11-a1d8-62ce26f2e5a4
📒 Files selected for processing (7)
bin/lib/onboard.jsdocs/reference/commands.mdsrc/lib/onboard-session.test.tssrc/lib/onboard-session.tssrc/lib/web-search.test.tssrc/lib/web-search.tstest/onboard.test.js
✅ Files skipped from review due to trivial changes (2)
- docs/reference/commands.md
- src/lib/onboard-session.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/web-search.test.ts
- test/onboard.test.js
- bin/lib/onboard.js
Author
|
Follow-up update in
Targeted validation that passed locally after this follow-up:
|
2 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/security/best-practices.md (2)
244-245: Avoid the superlative in the risk statement.“single most dangerous” reads as a superlative here. Spell out the concrete impact instead.
As per coding guidelines, "Superlatives and marketing language ... Say what it does, not how great it is." LLM pattern detected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/security/best-practices.md` around lines 244 - 245, The risk statement uses the superlative "single most dangerous"; replace it with a concrete impact description: update the "Risk if relaxed" line for `/sandbox/.openclaw` to explain that making `/sandbox/.openclaw` writable lets an agent replace wrapper symlinks and redirect config-backed paths to attacker-controlled locations, which can enable arbitrary code execution, persistence, or privilege escalation, rather than using the phrase "single most dangerous". Ensure the revised sentence appears where `/sandbox/.openclaw` and the policy file are mentioned and retains the original examples (wrapper symlinks, config-backed paths).
376-376: Format the env assignment and config key as inline code.
NEMOCLAW_DISABLE_DEVICE_AUTH=1andgateway.controlUi.dangerouslyDisableDeviceAuthshould be backticked in this table cell.As per coding guidelines, "CLI commands, file paths, flags, parameter names, and values must use inline
codeformatting."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/security/best-practices.md` at line 376, Update the table cell so the environment variable and config key are formatted as inline code: wrap NEMOCLAW_DISABLE_DEVICE_AUTH=1 and gateway.controlUi.dangerouslyDisableDeviceAuth in backticks so they appear as `NEMOCLAW_DISABLE_DEVICE_AUTH=1` and `gateway.controlUi.dangerouslyDisableDeviceAuth` respectively, ensuring CLI/flag and config key formatting follows the project's coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 1135-1139: The code always clears apiKey and usingSavedKey after
promptWebSearchRecovery(provider, validation), which causes transport-retry
actions (e.g., "retry") to lose the current key; change the logic to only clear
apiKey/usingSavedKey when the user explicitly chose to provide a new key (check
for the specific return value that indicates entering a new key, not "retry"),
leaving apiKey/usingSavedKey intact for "retry" and other non-key-change
actions; update the branch around promptWebSearchRecovery and its handling of
action (referencing promptWebSearchRecovery, action, apiKey, usingSavedKey,
provider, validation) accordingly.
- Around line 1231-1240: The current flow returns null for both "user answered
no" and "provider skipped", which prevents resume logic from seeing an explicit
disabled state; update the code paths in the onboarding flow around
prompt/isAffirmativeAnswer and promptWebSearchProviderChoice so that when the
user explicitly declines web search you return an object indicating
fetchEnabled: false (e.g., { fetchEnabled: false }) instead of null, and
likewise treat an explicit provider-skip decision by returning the same explicit
disabled state; ensure other code that expects the previous structure (resume
logic that checks fetchEnabled) continues to work with this explicit flag.
In `@scripts/nemoclaw-start.sh`:
- Around line 112-132: verify_config_layout() currently only validates the
openclaw.json file; extend it to perform the same full `.openclaw` wrapper
validation done in the root branch so non-root starts also validate
agents/hooks/skills/etc. Specifically, inside verify_config_layout() (and
similarly where the function is called at the other location referenced), after
validating OPENCLAW_CONFIG_SYMLINK/OPENCLAW_CONFIG_PATH, iterate the expected
wrapper entries (e.g., "agents","hooks","skills","secrets","runtime" — match the
names used in the existing root-only symlink scan) and for each ensure the
wrapper contains a symlink (use -L) whose resolved target (readlink -f) points
into the expected config area; on any mismatch or missing file emit the same
"[SECURITY] OpenClaw config ..." error and return non-zero so non-root runs fail
the same checks as root.
---
Nitpick comments:
In `@docs/security/best-practices.md`:
- Around line 244-245: The risk statement uses the superlative "single most
dangerous"; replace it with a concrete impact description: update the "Risk if
relaxed" line for `/sandbox/.openclaw` to explain that making
`/sandbox/.openclaw` writable lets an agent replace wrapper symlinks and
redirect config-backed paths to attacker-controlled locations, which can enable
arbitrary code execution, persistence, or privilege escalation, rather than
using the phrase "single most dangerous". Ensure the revised sentence appears
where `/sandbox/.openclaw` and the policy file are mentioned and retains the
original examples (wrapper symlinks, config-backed paths).
- Line 376: Update the table cell so the environment variable and config key are
formatted as inline code: wrap NEMOCLAW_DISABLE_DEVICE_AUTH=1 and
gateway.controlUi.dangerouslyDisableDeviceAuth in backticks so they appear as
`NEMOCLAW_DISABLE_DEVICE_AUTH=1` and
`gateway.controlUi.dangerouslyDisableDeviceAuth` respectively, ensuring CLI/flag
and config key formatting follows the project's coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b7c3881-99ff-4927-9e34-bd0c1fce4b67
📒 Files selected for processing (10)
DockerfileDockerfile.basebin/lib/onboard.jsdocs/reference/commands.mddocs/security/best-practices.mdnemoclaw-blueprint/policies/openclaw-sandbox.yamlscripts/nemoclaw-start.shtest/e2e-gateway-isolation.shtest/nemoclaw-start.test.jstest/openclaw-config-layout.test.js
✅ Files skipped from review due to trivial changes (2)
- nemoclaw-blueprint/policies/openclaw-sandbox.yaml
- test/openclaw-config-layout.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/reference/commands.md
Author
|
Addressed the remaining CodeRabbit follow-ups in
Validation:
|
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 (2)
bin/lib/onboard.js (2)
947-960:⚠️ Potential issue | 🟠 MajorReused sandboxes won't pick up web-search changes.
By limiting this sync script to
~/.nemoclaw/config.json, the only remaining place that applieswebSearchConfigis the Dockerfile patch during sandbox creation.createSandbox()returns early for an existing ready sandbox, so rerunning onboarding with web search newly enabled or with a different provider leaves the previous live OpenClaw web-search settings in place until the sandbox is recreated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 947 - 960, Reused sandboxes don't pick up web-search changes because buildSandboxConfigSyncScript only writes ~/.nemoclaw/config.json while webSearchConfig is applied only in the Dockerfile at creation; when createSandbox() returns early for an existing sandbox the Dockerfile patch is skipped. Update buildSandboxConfigSyncScript to also write the live OpenClaw web-search config into the sandbox (the same file/path the Dockerfile patch would have created) so webSearchConfig is synchronized on reuse, and/or modify createSandbox to call the sync script even when reusing an existing ready sandbox; reference buildSandboxConfigSyncScript, createSandbox, ~/.nemoclaw/config.json and the Dockerfile webSearchConfig patch when making the change.
4233-4249:⚠️ Potential issue | 🟠 MajorDon't silently disable web search on non-interactive resume.
When
ensureValidatedWebSearchCredential()returnsnullhere in--non-interactivemode, this branch falls back toconfigureWebSearch(null). If no explicit web-search env override is set, that returnsnull, overwritessession.webSearchConfig, and onboarding completes with web search disabled instead of failing the run.💡 Suggested guard
if (apiKey) { webSearchConfig = persistedWebSearchConfig; onboardSession.updateSession((current) => { current.webSearchConfig = webSearchConfig; return current; }); note(` [resume] Reusing ${label} configuration.`); } else { + if (isNonInteractive() && !resolveNonInteractiveWebSearchProvider()) { + console.error( + ` ${label} configuration could not be revalidated in non-interactive mode.`, + ); + process.exit(1); + } webSearchConfig = await configureWebSearch(null); onboardSession.updateSession((current) => { current.webSearchConfig = webSearchConfig; return current; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 4233 - 4249, On non-interactive resume, the code currently calls configureWebSearch(null) when ensureValidatedWebSearchCredential(...) returns null, which can overwrite session.webSearchConfig with null and silently disable web search; change the logic in this branch so that if running in non-interactive/resume mode and ensureValidatedWebSearchCredential(persistedWebSearchConfig.provider) returns null, you do NOT call configureWebSearch(null) or overwrite onboardSession.webSearchConfig—instead fail the run (throw or process.exit with a clear error) unless an explicit environment override is present; update the block handling persistedWebSearchConfig, referencing ensureValidatedWebSearchCredential, configureWebSearch, persistedWebSearchConfig and onboardSession.updateSession to implement this guard.
🧹 Nitpick comments (2)
scripts/nemoclaw-start.sh (2)
214-214: Minor: fallback path uses symlink location instead of actual config path.The default fallback
/sandbox/.openclaw/openclaw.jsonis the symlink, whileOPENCLAW_CONFIG_PATHpoints to the actual file at/sandbox/.openclaw-data/config/openclaw.json. Functionally equivalent since the symlink resolves correctly, but inconsistent with the new layout semantics.🔧 Align fallback with actual config path
-path = os.environ.get('OPENCLAW_CONFIG_PATH', '/sandbox/.openclaw/openclaw.json') +path = os.environ.get('OPENCLAW_CONFIG_PATH', '/sandbox/.openclaw-data/config/openclaw.json')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` at line 214, The fallback for OPENCLAW_CONFIG_PATH uses the symlink path; update the default in the os.environ.get call so the 'path' variable falls back to the actual config location (/sandbox/.openclaw-data/config/openclaw.json) instead of /sandbox/.openclaw/openclaw.json; modify the os.environ.get('OPENCLAW_CONFIG_PATH', ...) expression in the assignment to 'path' accordingly.
164-176: Consider warning on unexpected non-symlink entries for defense-in-depth.Line 165 silently skips non-symlink entries. While the immutable flag and DAC prevent runtime tampering, a warning would help detect build-time misconfigurations or unexpected artifacts.
🔒 Optional hardening to warn on non-symlink entries
for entry in "${OPENCLAW_STATE_DIR}"/*; do - [ -L "$entry" ] || continue + if [ ! -L "$entry" ]; then + echo "[SECURITY WARNING] Unexpected non-symlink in wrapper directory: ${entry}" >&2 + continue + fi name="$(basename "$entry")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 164 - 176, The loop over entries in OPENCLAW_STATE_DIR silently skips non-symlink files, which hides potential build-time misconfigurations; update the loop that currently checks [ -L "$entry" ] || continue to instead emit a warning to stderr (including the entry path and context like ${OPENCLAW_STATE_DIR}) when an entry is not a symlink and then continue, referencing the OPENCLAW_STATE_DIR, OPENCLAW_WRAPPER_ENTRIES, and openclaw_data_dir variables so reviewers can find and verify the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 947-960: Reused sandboxes don't pick up web-search changes because
buildSandboxConfigSyncScript only writes ~/.nemoclaw/config.json while
webSearchConfig is applied only in the Dockerfile at creation; when
createSandbox() returns early for an existing sandbox the Dockerfile patch is
skipped. Update buildSandboxConfigSyncScript to also write the live OpenClaw
web-search config into the sandbox (the same file/path the Dockerfile patch
would have created) so webSearchConfig is synchronized on reuse, and/or modify
createSandbox to call the sync script even when reusing an existing ready
sandbox; reference buildSandboxConfigSyncScript, createSandbox,
~/.nemoclaw/config.json and the Dockerfile webSearchConfig patch when making the
change.
- Around line 4233-4249: On non-interactive resume, the code currently calls
configureWebSearch(null) when ensureValidatedWebSearchCredential(...) returns
null, which can overwrite session.webSearchConfig with null and silently disable
web search; change the logic in this branch so that if running in
non-interactive/resume mode and
ensureValidatedWebSearchCredential(persistedWebSearchConfig.provider) returns
null, you do NOT call configureWebSearch(null) or overwrite
onboardSession.webSearchConfig—instead fail the run (throw or process.exit with
a clear error) unless an explicit environment override is present; update the
block handling persistedWebSearchConfig, referencing
ensureValidatedWebSearchCredential, configureWebSearch, persistedWebSearchConfig
and onboardSession.updateSession to implement this guard.
---
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Line 214: The fallback for OPENCLAW_CONFIG_PATH uses the symlink path; update
the default in the os.environ.get call so the 'path' variable falls back to the
actual config location (/sandbox/.openclaw-data/config/openclaw.json) instead of
/sandbox/.openclaw/openclaw.json; modify the
os.environ.get('OPENCLAW_CONFIG_PATH', ...) expression in the assignment to
'path' accordingly.
- Around line 164-176: The loop over entries in OPENCLAW_STATE_DIR silently
skips non-symlink files, which hides potential build-time misconfigurations;
update the loop that currently checks [ -L "$entry" ] || continue to instead
emit a warning to stderr (including the entry path and context like
${OPENCLAW_STATE_DIR}) when an entry is not a symlink and then continue,
referencing the OPENCLAW_STATE_DIR, OPENCLAW_WRAPPER_ENTRIES, and
openclaw_data_dir variables so reviewers can find and verify the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8530329f-7048-4505-8ebf-a4139055b149
📒 Files selected for processing (5)
bin/lib/onboard.jsdocs/security/best-practices.mdscripts/nemoclaw-start.shtest/nemoclaw-start.test.jstest/onboard.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/nemoclaw-start.test.js
- docs/security/best-practices.md
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
1 participant
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
#1464web-search onboarding path from Brave-only to a provider-aware flow for Brave, Gemini, and TavilywebSearchConfigstate across resumeopenclaw config set, Control UI edits, and direct config updates persist after onboardingLinked Issues
Related Issues
Why
#1464solved the Brave path, but the resulting flow was still provider-specific end to end: onboarding prompts, session state, Dockerfile config generation, and policy suggestions all assumed Brave. It also only worked around one symptom of#719by avoiding a web-search config write during onboarding.The sandbox still baked
openclaw.jsoninto a root-owned, read-only path, so later config changes for allowed origins, messaging channels, and other runtime settings still failed withEACCES. This PR now fixes both gaps: the provider-general onboarding flow and the underlying sandbox config mutability problem.Changes
src/lib/web-search.tsprovider-aware and emit full OpenClaw web-search config fragments instead of a Brave-only payloadgeminiandtavilypolicy presets and suggest the matching preset during onboardingwebSearchConfigsession persistence, explicit disable handling, invalid-provider normalization, and provider-aware credential redaction/sandbox/.openclaw-data/config/openclaw.json, expose it at/sandbox/.openclaw/openclaw.json, and exportOPENCLAW_CONFIG_PATHso runtime writes land on the writable path.openclawas the hardened wrapper path and replace the old immutable-config hash check with startup validation of the wrapper/symlink layoutDEFAULT_GEMINI_WEB_SEARCH_MODELin the generated Gemini web-search config fragment to match the documented OpenClaw schemaFollow-up Improvements
webSearchconfigType of Change
Validation
npm run build:clinpm run typecheck:cliRisks / Notes
/sandbox/.openclawitself locked as a root-owned wrapper with fixed symlinks so the config path cannot be swapped at runtimehadolintand nestednemoclaw/ESLint dependencies, so the final commit/push used--no-verifyafter the validation above passedSummary by CodeRabbit
New Features
Documentation
Policies
Tests