Skip to content

fix(onboard): don't abort onboard when Brave Search key validation fails - #2511

Merged
cv merged 2 commits into
mainfrom
fix/2507-brave-validation-non-fatal
Apr 29, 2026
Merged

fix(onboard): don't abort onboard when Brave Search key validation fails#2511
cv merged 2 commits into
mainfrom
fix/2507-brave-validation-non-fatal

Conversation

@laitingsheng

@laitingsheng laitingsheng commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Summary

nemoclaw onboard --non-interactive aborted with exit 1 whenever the Brave Web Search API key validation failed (HTTP 429 / 403 / network error / invalid key), even though Brave Web Search is an optional integration. Users were left with provider/gateway/inference partially configured but no sandbox. Downgrade the failure to a warning and skip the integration so the wizard continues to steps [5/8]–[8/8] and exits 0 if everything else succeeds.

Related issues

Closes #2507

Changes

  • src/lib/onboard.ts: in the non-interactive branch of configureWebSearch, replace process.exit(1) on validateBraveSearchApiKey failure with console.warn + return null. The warning points users at `nemoclaw config web-search` to re-enable the integration later. Matches the existing "no web search" return contract already used elsewhere in the function.
  • test/onboard-brave-validation.test.ts: regression test using the curl-shim + spawned-Node-script pattern from test/onboard-selection.test.ts. Covers (a) HTTP 429 -> result is null, no process.exit calls, warning mentions the recovery command; (b) HTTP 200 -> result is { fetchEnabled: true }.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • make docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

AI Disclosure

  • AI-assisted — tool: Claude Code

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Brave Web Search configuration now warns and disables web search on invalid API keys, and provides instructions to re-enable, instead of terminating the process.
  • Tests
    • Added a non-interactive integration test validating Brave API key handling for success and rate-limit scenarios.

## Summary

`nemoclaw onboard --non-interactive` aborted with exit 1 whenever the
Brave Web Search API key validation failed (HTTP 429 / 403 / network
error / invalid key), even though Brave Web Search is an optional
integration. Users were left with provider/gateway/inference partially
configured but no sandbox. Downgrade the failure to a warning and skip
the integration so the wizard continues to steps [5/8]–[8/8] and exits 0
if everything else succeeds.

## Related issues

Closes #2507

## Changes

- src/lib/onboard.ts: in the non-interactive branch of
  configureWebSearch, replace process.exit(1) on
  validateBraveSearchApiKey failure with console.warn + return null.
  The warning points users at \`nemoclaw config web-search\` to
  re-enable the integration later. Matches the existing "no web search"
  return contract already used elsewhere in the function.
- test/onboard-brave-validation.test.ts: regression test using the
  curl-shim + spawned-Node-script pattern from
  test/onboard-selection.test.ts. Covers (a) HTTP 429 -> result is
  null, no process.exit calls, warning mentions the recovery command;
  (b) HTTP 200 -> result is { fetchEnabled: true }.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@coderabbitai

coderabbitai Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d6bd29c2-432f-4887-b332-b2c077d22c26

📥 Commits

Reviewing files that changed from the base of the PR and between a4227f5 and 990cd2c.

📒 Files selected for processing (1)
  • src/lib/onboard.ts

📝 Walkthrough

Walkthrough

Non-interactive Brave Web Search API key validation now emits warnings and returns null (disabling web search) on failure instead of exiting the process. A new integration-style test runs the onboard logic in a subprocess with a shimmed curl to validate both failure (HTTP 429) and success (HTTP 200) behaviors.

Changes

Cohort / File(s) Summary
Brave validation runtime
src/lib/onboard.ts
Non-interactive Brave API key validation failures no longer call process.exit(1); they log warnings (including guidance to re-enable via nemoclaw config web-search) and return null to disable web search.
Brave validation tests
test/onboard-brave-validation.test.ts
Added integration-style test that executes compiled onboard code in a subprocess with NEMOCLAW_NON_INTERACTIVE=1, a shimmed curl, and temporary env; asserts behavior for HTTP 429 (result null, warning present, no exit) and HTTP 200 (fetch enabled, no exit).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I sniffed the keys and gave a thump,
A warning hop — no sudden dump.
Web search sleeps, the onboarding goes,
I nibble logs where calm wind blows. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: the PR prevents onboard abort when Brave Search API key validation fails, which is the core issue being addressed.
Linked Issues check ✅ Passed The PR fully addresses issue #2507 requirements: (1) non-interactive onboard no longer aborts on Brave validation failure [2507]; (2) warning includes failure reason and recovery command [2507]; (3) onboarding continues to steps [5/8]–[8/8] [2507]; (4) exits 0 when other steps succeed [2507]; (5) skips Brave Web Search enabling when validation fails [2507].
Out of Scope Changes check ✅ Passed All changes are strictly within scope of issue #2507: onboard.ts changes implement the validation-failure-to-warning downgrade, and the new test validates the fix with curl-shimming and non-interactive execution patterns.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/2507-brave-validation-non-fatal

Review rate limit: 7/10 reviews remaining, refill in 17 minutes and 50 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@test/onboard-brave-validation.test.ts`:
- Around line 45-48: Replace the hardcoded ":" PATH separator usages in the test
with Node's cross-platform path.delimiter: update the PATH construction that
uses fakeBin + ":" + process.env.PATH (and the similar concatenations around
tmpDir/bin at the other two occurrences) to join with path.delimiter instead;
locate these string concatenations near the variables repoRoot, tmpDir, fakeBin
and scriptPath in onboard-brave-validation.test.ts and swap ":" for
path.delimiter so the test works on Windows and POSIX CI runners.
🪄 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: f14d26e4-3910-4cfa-abf0-348785c647db

📥 Commits

Reviewing files that changed from the base of the PR and between 6f7f0c6 and a4227f5.

📒 Files selected for processing (2)
  • src/lib/onboard.ts
  • test/onboard-brave-validation.test.ts

Comment thread test/onboard-brave-validation.test.ts
@wscurran wscurran added bug platform: ubuntu Affects Ubuntu Linux environments integration: brave Brave integration behavior labels Apr 27, 2026
@wscurran

Copy link
Copy Markdown
Contributor

✨ Thanks for submitting this pull request that proposes a way to fix a bug where the onboard process aborts when Brave Search key validation fails.


Related open issues:

@cv
cv merged commit 9bfe30b into main Apr 29, 2026
18 checks passed
@miyoungc miyoungc mentioned this pull request Apr 30, 2026
13 tasks
miyoungc added a commit that referenced this pull request Apr 30, 2026
## Summary
Refreshes the daily docs from NemoClaw commits merged in the past 24
hours and advances the docs metadata from 0.0.29 to 0.0.31, the next
version after tag v0.0.30.
The updates cover documented behavior gaps found in the merged PRs
listed below.

## Related Issue
None.

## Changes
- `docs/versions1.json` and `docs/project.json`: bump the preferred docs
version to `0.0.31` for daily release preparation after latest tag
`v0.0.30`.
- `docs/reference/commands.md`: document non-interactive Brave Search
validation fallback from #2511 / 9bfe30b, missing `--from <Dockerfile>`
path validation from #2597 / 7186834, and `logs` reading OpenShell
audit events from #2590 / e225dfb.
- `docs/inference/use-local-inference.md`: document local inference
reachability retry and host-side fallback from #2453 / 9dbe855, plus
compatible-endpoint timeout coverage from #2583 / b4ef3db.
- `docs/reference/troubleshooting.md`: document source-install shim
fallback from #2520 / 01a177c, TLS gateway trust recovery from #1936 /
24725d2, compatible-endpoint timeout coverage from #2583 / b4ef3db,
local reachability diagnostics from #2453 / 9dbe855, and host proxy
`NO_PROXY` injection from #2662 / b4df07e.

## Type of Change
- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Verification
- [ ] `npx prek run --all-files` passes
- [ ] `npm test` passes
- [ ] Tests added or updated for new or changed behavior
- [x] No secrets, API keys, or credentials committed
- [x] Docs updated for user-facing behavior changes
- [x] `make 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:
- `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix
nemoclaw-user --dry-run` passed.
- `git diff --check` passed.
- Pre-push hooks passed through markdownlint, docs-to-skills, JSON
checks, gitleaks, and version sync before `Test (skills YAML)` failed
because this fresh worktree lacked `vitest/config`.
- `npx prek run --all-files` could not run from the fresh worktree
because `npx prek` resolved to a missing `prek@*` package; downloading
`@j178/prek` was not approved.
- `npm test` could not complete from the fresh worktree because
dependencies and compiled `dist/lib/*` artifacts were absent.

## AI Disclosure
- [x] AI-assisted — tool: OpenAI Codex

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
  * Version updated to 0.0.31
* Local inference onboarding now includes retry logic for container
reachability checks
  * Web search setup failure handling clarified with fallback guidance
  * Dockerfile path validation timing documented
  * Logging behavior clarified for concurrent stream reading
  * New TLS/certificate troubleshooting section added
  * Install path and proxy configuration troubleshooting updated

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
DemianHeyGen pushed a commit to DemianHeyGen/NemoClaw that referenced this pull request Apr 30, 2026
…ils (NVIDIA#2511)

## Summary

`nemoclaw onboard --non-interactive` aborted with exit 1 whenever the
Brave Web Search API key validation failed (HTTP 429 / 403 / network
error / invalid key), even though Brave Web Search is an optional
integration. Users were left with provider/gateway/inference partially
configured but no sandbox. Downgrade the failure to a warning and skip
the integration so the wizard continues to steps [5/8]–[8/8] and exits 0
if everything else succeeds.

## Related issues

Closes NVIDIA#2507

## Changes

- src/lib/onboard.ts: in the non-interactive branch of
configureWebSearch, replace process.exit(1) on validateBraveSearchApiKey
failure with console.warn + return null. The warning points users at
\`nemoclaw config web-search\` to re-enable the integration later.
Matches the existing "no web search" return contract already used
elsewhere in the function.
- test/onboard-brave-validation.test.ts: regression test using the
curl-shim + spawned-Node-script pattern from
test/onboard-selection.test.ts. Covers (a) HTTP 429 -> result is null,
no process.exit calls, warning mentions the recovery command; (b) HTTP
200 -> result is { fetchEnabled: true }.

## Type of Change

- [X] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. -->
- [X] `npx prek run --all-files` passes
- [X] `npm test` passes
- [X] Tests added or updated for new or changed behavior
- [X] No secrets, API keys, or credentials committed
- [ ] Docs updated for user-facing behavior changes
- [ ] `make docs` builds without warnings (doc changes only)
- [ ] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

## AI Disclosure
<!-- If an AI agent authored or co-authored this PR, check the box and
name the tool. Remove this section for fully human-authored PRs. -->
- [X] AI-assisted — tool: Claude Code<!-- e.g., Claude Code, Cursor,
GitHub Copilot -->

---
<!-- DCO sign-off required by CI. Run: git config user.name && git
config user.email -->
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Brave Web Search configuration now warns and disables web search on
invalid API keys, and provides instructions to re-enable, instead of
terminating the process.
* **Tests**
* Added a non-interactive integration test validating Brave API key
handling for success and rate-limit scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
DemianHeyGen pushed a commit to DemianHeyGen/NemoClaw that referenced this pull request Apr 30, 2026
## Summary
Refreshes the daily docs from NemoClaw commits merged in the past 24
hours and advances the docs metadata from 0.0.29 to 0.0.31, the next
version after tag v0.0.30.
The updates cover documented behavior gaps found in the merged PRs
listed below.

## Related Issue
None.

## Changes
- `docs/versions1.json` and `docs/project.json`: bump the preferred docs
version to `0.0.31` for daily release preparation after latest tag
`v0.0.30`.
- `docs/reference/commands.md`: document non-interactive Brave Search
validation fallback from NVIDIA#2511 / 9bfe30b, missing `--from <Dockerfile>`
path validation from NVIDIA#2597 / 7186834, and `logs` reading OpenShell
audit events from NVIDIA#2590 / e225dfb.
- `docs/inference/use-local-inference.md`: document local inference
reachability retry and host-side fallback from NVIDIA#2453 / 9dbe855, plus
compatible-endpoint timeout coverage from NVIDIA#2583 / b4ef3db.
- `docs/reference/troubleshooting.md`: document source-install shim
fallback from NVIDIA#2520 / 01a177c, TLS gateway trust recovery from NVIDIA#1936 /
24725d2, compatible-endpoint timeout coverage from NVIDIA#2583 / b4ef3db,
local reachability diagnostics from NVIDIA#2453 / 9dbe855, and host proxy
`NO_PROXY` injection from NVIDIA#2662 / b4df07e.

## Type of Change
- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Verification
- [ ] `npx prek run --all-files` passes
- [ ] `npm test` passes
- [ ] Tests added or updated for new or changed behavior
- [x] No secrets, API keys, or credentials committed
- [x] Docs updated for user-facing behavior changes
- [x] `make 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:
- `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix
nemoclaw-user --dry-run` passed.
- `git diff --check` passed.
- Pre-push hooks passed through markdownlint, docs-to-skills, JSON
checks, gitleaks, and version sync before `Test (skills YAML)` failed
because this fresh worktree lacked `vitest/config`.
- `npx prek run --all-files` could not run from the fresh worktree
because `npx prek` resolved to a missing `prek@*` package; downloading
`@j178/prek` was not approved.
- `npm test` could not complete from the fresh worktree because
dependencies and compiled `dist/lib/*` artifacts were absent.

## AI Disclosure
- [x] AI-assisted — tool: OpenAI Codex

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
  * Version updated to 0.0.31
* Local inference onboarding now includes retry logic for container
reachability checks
  * Web search setup failure handling clarified with fallback guidance
  * Dockerfile path validation timing documented
  * Logging behavior clarified for concurrent stream reading
  * New TLS/certificate troubleshooting section added
  * Install path and proxy configuration troubleshooting updated

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
@wscurran wscurran added area: cli Command line interface, flags, terminal UX, or output area: integrations Third-party service integration behavior bug-fix PR fixes a bug or regression feature PR adds or expands user-visible functionality and removed NemoClaw CLI feature PR adds or expands user-visible functionality labels Jun 3, 2026
@wscurran wscurran added the NV QA Bugs found by the NVIDIA QA Team label Jun 26, 2026
@cv
cv deleted the fix/2507-brave-validation-non-fatal branch June 28, 2026 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output area: integrations Third-party service integration behavior bug-fix PR fixes a bug or regression integration: brave Brave integration behavior NV QA Bugs found by the NVIDIA QA Team platform: ubuntu Affects Ubuntu Linux environments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Ubuntu 22.04][Onboard] Brave Search API key validation failure aborts non-interactive onboard

3 participants