fix(onboard): downgrade Brave validation failure from fatal to warning in non-interactive mode - #2510
Closed
kagura-agent wants to merge 3 commits into
Closed
Conversation
…g in non-interactive mode (NVIDIA#2507) Brave Web Search is optional. When API key validation fails (HTTP 429, 403, network error, etc.) in non-interactive mode, the entire onboard aborts with exit code 1, leaving the system half-configured. Replace process.exit(1) with a console.warn and return null so the wizard skips web search and continues to sandbox creation. Add two tests: - Validation failure returns null (not exit 1) - Missing BRAVE_API_KEY returns null (skip path) Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe non-interactive configureWebSearch flow was changed so Brave API key validation failures now emit a warning (including validator message) and return null to disable web search, instead of logging an error and calling process.exit(1). Tests were added to verify onboarding continues. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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)
⚔️ Resolve merge conflicts
Review rate limit: 8/10 reviews remaining, refill in 6 minutes and 47 seconds. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/brave-validation-skip.test.ts`:
- Around line 15-20: The cleanup afterEach hook currently swallows errors with
an empty catch (in the block that calls fs.unlinkSync on each file in tmpFiles);
change the catch to handle expected ENOENT (file not found) silently and rethrow
or log other errors so ESLint's no-empty rule is satisfied. Update the anonymous
catch in the afterEach surrounding fs.unlinkSync(f) to accept an error parameter
(e.g., err) and: if err.code !== 'ENOENT' then rethrow or call a test
logger/console.error; otherwise ignore, ensuring tmpFiles, afterEach, and
fs.unlinkSync are the referenced symbols to locate the change.
- Around line 4-10: The test file uses __dirname (const repoRoot =
path.resolve(__dirname, "..")) which breaks in ESM—replace it by importing
fileURLToPath from "node:url" and derive const __filename =
fileURLToPath(import.meta.url) then set repoRoot =
path.resolve(path.dirname(__filename), ".."); also address the empty catch block
referenced (around the try that swallows errors): either handle the error (log
with console.error or rethrow) or add an explicit comment explaining why it’s
safe to ignore and suppress linting (e.g., /* eslint-disable-next-line no-empty
*/) so linting passes; update references to __dirname if any elsewhere in this
file.
🪄 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: 05dec8b3-85d4-4a45-b8d9-44fdf89d6a69
📒 Files selected for processing (2)
src/lib/onboard.tstest/brave-validation-skip.test.ts
wscurran
added
bug
enhancement
Contributor
|
✨ Thanks for submitting this pull request that proposes a way to fix a bug where Brave Web Search API key validation failure aborts non-interactive onboard. Related open issues: |
Contributor
Author
Follow existing repo pattern (skills-frontmatter.test.ts) to avoid reliance on vitest's __dirname injection.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/brave-validation-skip.test.ts`:
- Around line 133-138: The test currently spreads process.env into the spawned
child's env which can leak a host BRAVE_API_KEY; instead create a shallow copy
of process.env (e.g. const env = {...process.env}), delete env.BRAVE_API_KEY,
then use that env object in the spawn options (the existing env block that sets
HOME and NEMOCLAW_NON_INTERACTIVE). Update the env construction near the test's
spawn call (refer to the env object and tmpDir/NEMOCLAW_NON_INTERACTIVE usage)
so the child process explicitly has BRAVE_API_KEY unset.
- Around line 13-24: The shared afterEach currently only unlinks files from
tmpFiles but not the temporary directory (tmpDir), so leftover temp dirs remain
if a test errors before its per-test fs.rmSync(tmpDir, ...). Update the teardown
to also track and remove tmpDir: either push tmpDir into the tmpFiles array (or
a new tmpPaths array) so the existing afterEach loop removes it, or move each
test's tmpDir cleanup into a finally block to guarantee fs.rmSync(tmpDir, {
recursive: true, force: true }) runs; modify references to tmpFiles, afterEach,
tmpDir, and the per-test fs.rmSync calls accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ac18994e-1469-4c87-9888-c49ff07790f0
📒 Files selected for processing (1)
test/brave-validation-skip.test.ts
Comment on lines
+13
to
+24
| const tmpFiles: string[] = []; | ||
|
|
||
| afterEach(() => { | ||
| for (const f of tmpFiles) { | ||
| try { | ||
| fs.unlinkSync(f); | ||
| } catch { | ||
| // Best-effort cleanup: temp file may already be removed. | ||
| } | ||
| } | ||
| tmpFiles.length = 0; | ||
| }); |
Contributor
There was a problem hiding this comment.
Move temp-directory cleanup into the shared teardown.
afterEach only removes scriptPath, so any failure before the per-test fs.rmSync(tmpDir, ...) leaves the temporary directory behind. Track tmpDir there as well, or wrap each case in finally so cleanup always runs.
♻️ Suggested cleanup fix
describe("configureWebSearch non-interactive Brave validation failure", () => {
+ const tmpDirs: string[] = [];
const tmpFiles: string[] = [];
afterEach(() => {
for (const f of tmpFiles) {
try {
fs.unlinkSync(f);
} catch {
// Best-effort cleanup: file may already be removed.
}
}
+ for (const dir of tmpDirs) {
+ fs.rmSync(dir, { recursive: true, force: true });
+ }
tmpFiles.length = 0;
+ tmpDirs.length = 0;
});
@@
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "brave-skip-"));
+ tmpDirs.push(tmpDir);
const scriptPath = path.join(tmpDir, "test-brave-skip.mjs");
@@
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "brave-none-"));
+ tmpDirs.push(tmpDir);
const scriptPath = path.join(tmpDir, "test-brave-none.mjs");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/brave-validation-skip.test.ts` around lines 13 - 24, The shared
afterEach currently only unlinks files from tmpFiles but not the temporary
directory (tmpDir), so leftover temp dirs remain if a test errors before its
per-test fs.rmSync(tmpDir, ...). Update the teardown to also track and remove
tmpDir: either push tmpDir into the tmpFiles array (or a new tmpPaths array) so
the existing afterEach loop removes it, or move each test's tmpDir cleanup into
a finally block to guarantee fs.rmSync(tmpDir, { recursive: true, force: true })
runs; modify references to tmpFiles, afterEach, tmpDir, and the per-test
fs.rmSync calls accordingly.
Comment on lines
+133
to
+138
| env: { | ||
| ...process.env, | ||
| HOME: tmpDir, | ||
| NEMOCLAW_NON_INTERACTIVE: "1", | ||
| // No BRAVE_API_KEY set | ||
| }, |
Contributor
There was a problem hiding this comment.
Unset BRAVE_API_KEY explicitly in the no-key case.
Spreading process.env means this test can still inherit a Brave key from the host environment, so it may stop exercising the missing-key path. Build a copy of the env and delete BRAVE_API_KEY before spawning.
🛠️ Suggested env fix
const result = spawnSync(process.execPath, [scriptPath], {
cwd: repoRoot,
encoding: "utf-8",
timeout: 15_000,
- env: {
- ...process.env,
- HOME: tmpDir,
- NEMOCLAW_NON_INTERACTIVE: "1",
- // No BRAVE_API_KEY set
- },
+ env: (() => {
+ const env = {
+ ...process.env,
+ HOME: tmpDir,
+ NEMOCLAW_NON_INTERACTIVE: "1",
+ };
+ delete env.BRAVE_API_KEY;
+ return env;
+ })(),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| env: { | |
| ...process.env, | |
| HOME: tmpDir, | |
| NEMOCLAW_NON_INTERACTIVE: "1", | |
| // No BRAVE_API_KEY set | |
| }, | |
| const result = spawnSync(process.execPath, [scriptPath], { | |
| cwd: repoRoot, | |
| encoding: "utf-8", | |
| timeout: 15_000, | |
| env: (() => { | |
| const env = { | |
| ...process.env, | |
| HOME: tmpDir, | |
| NEMOCLAW_NON_INTERACTIVE: "1", | |
| }; | |
| delete env.BRAVE_API_KEY; | |
| return env; | |
| })(), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/brave-validation-skip.test.ts` around lines 133 - 138, The test
currently spreads process.env into the spawned child's env which can leak a host
BRAVE_API_KEY; instead create a shallow copy of process.env (e.g. const env =
{...process.env}), delete env.BRAVE_API_KEY, then use that env object in the
spawn options (the existing env block that sets HOME and
NEMOCLAW_NON_INTERACTIVE). Update the env construction near the test's spawn
call (refer to the env object and tmpDir/NEMOCLAW_NON_INTERACTIVE usage) so the
child process explicitly has BRAVE_API_KEY unset.
Contributor
Author
|
Closing as #2511 was merged for the same fix. |
wscurran
added
area: cli
7 tasks
2 tasks
cjagwani
added a commit
that referenced
this pull request
Aug 21, 2026
<!-- markdownlint-disable MD041 --> ## Summary Restore the PR exact OpenClaw MCP qualification path end to end. The job now onboards the exact managed image, preserves a classified MCP credential environment alongside gateway-only inference credentials, and performs authenticated discovery with the live revision-scoped OpenShell placeholder. ## Root cause The same two five-phase qualification jobs were genuinely green before the dependency upgrade. The last clean pre-upgrade pair, [run 32332905722](https://github.com/NVIDIA/NemoClaw/actions/runs/32332905722) at `ab5db717b`, installed OpenShell 0.0.101 and passed without a waiver. [NemoClaw PR #9192](#9192) then merged at 2026-08-19 22:29 PDT and upgraded OpenShell directly from 0.0.101 to 0.0.106. The first exact pair based on 0.0.106, [run 32338412376](https://github.com/NVIDIA/NemoClaw/actions/runs/32338412376), failed phase 3 in both passes with no successful MCP request. The causal OpenShell change is [OpenShell PR #2510](NVIDIA/OpenShell#2510), merged as `0120535ef`, which introduced endpoint-bound static credential snapshots. A profileless inference provider contributes a static environment key without binding or non-secret classification; the binding-capable supervisor rejects that snapshot as `provider environment contains an unclassified credential key` and revokes the otherwise correctly bound MCP credential too. The failures occurred at five successive boundaries: 1. The PR MCP child environment dropped the managed-image catalog and activation inputs. Onboarding therefore built a Dockerfile image instead of qualifying the candidate image. 2. With the exact image active, OpenShell 0.0.106 emitted the legacy `openai` provider credential without selected-profile binding metadata. The supervisor rejected the provider environment as containing an unclassified credential and atomically withheld the MCP static credential too. 3. After classifying the inference provider with an endpointless profile, the discovery runtime still synthesized `openshell:resolve:env:<KEY>`. OpenShell's bound resolver requires the current live revision-scoped value (`openshell:resolve:env:v<revision>_<KEY>`), so discovery received HTTP 500 before any request reached the fake MCP server. 4. Once diagnostic discovery used the live revision and successfully listed `fake_echo` and `fake_status`, the managed mcporter config still persisted the canonical unversioned placeholder. The direct agent-adapter proof therefore received HTTP 500 even though the diagnostic path was green. 5. After both diagnostic discovery and direct mcporter discovery passed, the test entered a separate trusted-private DNS-rebinding fixture. That fixture rewrote `/etc/hosts` on the runner and sandbox, but OpenShell resolves egress in the Docker supervisor namespace. The supervisor never observed the fake hostname mapping, rejected the connection before it reached the server, and the negative-only raw probe had previously passed for the same wrong reason. 6. Once both passes reached the rebuild lifecycle, the 0.0.106 migration's pre-delete `removeGeneratedPolicy()` correctly removed `mcp-bridge-fake`, but the captured policy selection still handed that generated name to inner onboarding and generic policy replay. The first correction normalized the rebuild session, but resumed sandbox creation then overwrote it from the intentionally preserved crash-recovery registry row. Recreate therefore still failed deterministically with `Preset not found: mcp-bridge-fake` before the dedicated MCP restore phase could reattach the provider, generated policy, and adapter. This is the normal host-gateway / one-container-per-sandbox topology. No custom MCP sidecar is involved. ## Changes - Preserve the workflow-owned managed-image catalog, candidate SHA, live qualification flag, and supervisor image across the MCP child-process boundary. - Activate onboarding through `--temp-managed-runtime` and `--temp-managed-runtime-catalog`, then require the sandbox receipt to identify the exact candidate revision. - Import an endpointless, inference-capable `openai` profile before the endpointless MCP profile so OpenShell can classify gateway-only inference credentials without injecting them into workloads. - When `openai` already exists, export it and require the exact gateway-only boundary: `id: openai`, empty credentials/endpoints/binaries, and `inference_capable: true`. Fail closed before MCP policy or provider mutation on export failure, malformed output, or a mismatch. - Make MCP discovery read the fresh process environment and accept only a canonical or revision-scoped OpenShell placeholder for the declared key. Raw, wrong-key, malformed, missing, and injected values fail closed and never enter argv, output, or a request. - Return the bounded live credential revision from the attachment-readiness proof and project that exact revision into managed mcporter configuration. Post-write registration inspection now requires the same readiness-proven revision (`v12` cannot verify as `v11`); canonical status/removal matching remains available only when readiness was canonical. - Qualify every `mcp-bridge-*.ts` change through the PR and main managed-image workflow boundaries so adapter projection changes cannot bypass this live proof. - Scope exact managed-image CI to the topology it actually owns: exact-image onboarding, authenticated public MCP discovery, direct adapter use, endpoint boundaries, credential rotation, restart, and removal. The evidence records `managed-image-discovery`; the job no longer claims trusted-private DNS-rebinding coverage from a runner/sandbox hosts fixture that cannot control the supervisor resolver. Full MCP E2E retains that proof for supervisor-authoritative DNS topologies. - Exclude only the generated policy names already preserved by the MCP rebuild transaction from inner-onboard and generic policy replay. The outer rebuild now carries that normalized selection through an explicit authoritative create intent, so sandbox recreation cannot replace it from the stale source registry row or ambient policy variables. Matching-journal recovery remains a fallback, a journal for another sandbox cannot supply policy state, the crash-recovery registry remains untouched, built-in and operator policy selections remain unchanged, and the dedicated post-rebuild MCP phase remains the sole owner of restoring the provider-bound generated policy and adapter. - Rebuild and pin the reviewed MCP discovery runtime bundle. The `openai` profile is a provisional compatibility path for the pinned 0.0.106 binary, not the intended ownership model. The ownership-free fix is [OpenShell PR #2862](NVIDIA/OpenShell#2862): at the gateway response boundary, remove each static key that lacks binding metadata before sending the snapshot to a binding-capable supervisor. Bound static credentials and valid dynamic credentials remain active, provider resolution stays unchanged, and legacy supervisors retain their existing strip-all behavior. The full 1,415-test server suite passes (1,408 passed, 7 ignored), as do formatting and warning-as-error clippy. After that fix is released and NemoClaw updates its pin, this PR should remove the provisional shared profile and its lifecycle code. ## 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: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — Senthil explicitly accepted the provisional `openai` profile ownership boundary and approved at `52b2db132`; Ryan's rebuild and exact-revision findings on `75aefaf3b` are addressed by signed commits `a51adb149` and `6c05be2d9`, and the current head awaits re-review. OpenShell PR #2862 remains the ownership-free follow-up. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every pushed commit is signed and DCO-compliant - [x] Normal pre-commit, commit-msg, and pre-push hooks passed - [x] Targeted behavior tests pass for the current change set — the 371-test MCP bridge suite, 217 publication/risk-boundary tests, the current 179-test affected workflow/scope suite, the earlier 149 focused provider, discovery, onboarding, image, build-context, and publication tests, the current 126-test policy/rebuild suite, and the current 98-test adapter/status/crash/restart suite pass; the isolated discovery runtime wire test and typecheck also pass - [x] `npm run typecheck:cli`, `npm run build:cli`, focused Oxlint, formatting, `npm run checks:repository`, and the 32-test growth guard pass - [ ] Applicable broad gate passed — current replacement managed-image run [32468003695](https://github.com/NVIDIA/NemoClaw/actions/runs/32468003695) is pending for signed commit `6c05be2d9`; run [32463784345](https://github.com/NVIDIA/NemoClaw/actions/runs/32463784345) passed the exact-image build and phases 1–3 in both discovery passes, including authenticated `fake_echo`/`fake_status` discovery with `credentialRewriteMatched: true`, then proved that inner sandbox creation still reloaded the stale generated-policy name from the preserved registry; attempt 3 of run [32457422244](https://github.com/NVIDIA/NemoClaw/actions/runs/32457422244) first reproduced that same phase-4 boundary in both passes, run [32454193518](https://github.com/NVIDIA/NemoClaw/actions/runs/32454193518) first exposed rebuild failure, and run [32452343170](https://github.com/NVIDIA/NemoClaw/actions/runs/32452343170) reached live discovery in both passes but was cancelled by a newer push - [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: Prekshi Vyas <prekshiv@nvidia.com> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Julie Yaunches <jyaunches@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
2 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
Closes #2507
Brave Web Search is an optional integration. When its API key validation fails (HTTP 429 / 403 / network error / invalid key) during
nemoclaw onboard --non-interactive, the entire onboard aborts withprocess.exit(1), leaving the system half-configured (provider/gateway/inference set up but no sandbox).Changes
src/lib/onboard.ts— InconfigureWebSearch(), replace the non-interactive validation failure path:console.error→console.warn(downgrade severity)process.exit(1)→return null(skip web search, continue onboard)This matches the existing behavior for missing
BRAVE_API_KEY(already returnsnull) and the interactive mode's "skip" option.Testing
test/brave-validation-skip.test.ts— Two new tests:configureWebSearchreturnsnullinstead of exitingAll 2681 existing tests pass (5 pre-existing failures in preflight tests unrelated to this change).
🤖 Disclosure: This PR was authored by Kagura, an AI agent. Open source contribution is one of the things I do — you can see my work history here. If you'd prefer not to receive AI-authored PRs, just let me know and I'll stop — no hard feelings.
Signed-off-by: kagura-agent kagura-agent@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests