Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/lib/onboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1390,11 +1390,11 @@ async function configureWebSearch(
note(" [non-interactive] Brave Web Search requested.");
const validation = validateBraveSearchApiKey(braveApiKey);
if (!validation.ok) {
console.error(" Brave Search API key validation failed.");
console.warn(" Brave Search API key validation failed — web search will be disabled.");
if (validation.message) {
console.error(` ${validation.message}`);
console.warn(` ${validation.message}`);
}
process.exit(1);
return null;
}
saveCredential(webSearch.BRAVE_API_KEY_ENV, braveApiKey);
process.env[webSearch.BRAVE_API_KEY_ENV] = braveApiKey;
Expand Down
150 changes: 150 additions & 0 deletions test/brave-validation-skip.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

import { spawnSync } from "node:child_process";
import fs from "node:fs";
import os from "node:os";
import path from "node:path";
import { fileURLToPath } from "node:url";
import { describe, it, expect, afterEach } from "vitest";

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
const repoRoot = path.resolve(__dirname, "..");
Comment thread
coderabbitai[bot] marked this conversation as resolved.

describe("configureWebSearch non-interactive Brave validation failure", () => {
const tmpFiles: string[] = [];

afterEach(() => {
for (const f of tmpFiles) {
try {
fs.unlinkSync(f);
} catch {
// Best-effort cleanup: temp file may already be removed.
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
tmpFiles.length = 0;
});
Comment on lines +13 to +24

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.

⚠️ Potential issue | 🟡 Minor

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.


it("returns null instead of exiting when Brave API key validation fails", () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "brave-skip-"));
const scriptPath = path.join(tmpDir, "test-brave-skip.mjs");
tmpFiles.push(scriptPath);

// Script that imports configureWebSearch with mocked runCurlProbe
const script = `
import { createRequire } from "node:module";
const require = createRequire(import.meta.url);
const Module = require("module");

// Intercept require to mock runCurlProbe
const origResolve = Module._resolveFilename;
Module._resolveFilename = function(request, parent, isMain, options) {
return origResolve.call(this, request, parent, isMain, options);
};

// We need to patch the onboard module after loading.
// Load the compiled module and call configureWebSearch.
const onboard = require("${repoRoot.replace(/\\/g, "/")}/dist/lib/onboard.js");

// Mock the validation function by patching the module-level function.
// configureWebSearch calls validateBraveSearchApiKey internally,
// which calls runCurlProbe which calls spawnSync(curl, ...).
// We mock spawnSync at the child_process level.
const cp = require("node:child_process");
const origSpawnSync = cp.spawnSync;
cp.spawnSync = function(cmd, args, opts) {
// When curl is called for Brave validation, return a 429 error
if (cmd === "curl" && args && args.some(a => typeof a === "string" && a.includes("brave.com"))) {
return {
status: 0,
stdout: '{"type":"ErrorResponse","error":{"status":429,"detail":"Rate limit exceeded"}}',
stderr: "",
};
}
return origSpawnSync.call(this, cmd, args, opts);
};

async function main() {
const result = await onboard.configureWebSearch(null);
// If we reach here, process.exit was NOT called
console.log("RESULT:" + JSON.stringify(result));
process.exit(0);
}

main().catch(err => {
console.error("ERROR:" + err.message);
process.exit(2);
});
`;

fs.writeFileSync(scriptPath, script);

const result = spawnSync(process.execPath, [scriptPath], {
cwd: repoRoot,
encoding: "utf-8",
timeout: 15_000,
env: {
...process.env,
HOME: tmpDir,
BRAVE_API_KEY: "test-invalid-key-12345",
NEMOCLAW_NON_INTERACTIVE: "1",
},
});

// Should exit 0, not 1
expect(result.status).toBe(0);

// Should return null (skip web search)
expect(result.stdout).toContain("RESULT:null");

// Should print a warning about validation failure
expect(result.stderr).toContain("Brave Search API key validation failed");

// Cleanup tmpDir
fs.rmSync(tmpDir, { recursive: true, force: true });
});

it("skips Brave web search when no BRAVE_API_KEY is set", () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "brave-none-"));
const scriptPath = path.join(tmpDir, "test-brave-none.mjs");
tmpFiles.push(scriptPath);

const script = `
import { createRequire } from "node:module";
const require = createRequire(import.meta.url);
const onboard = require("${repoRoot.replace(/\\/g, "/")}/dist/lib/onboard.js");

async function main() {
const result = await onboard.configureWebSearch(null);
console.log("RESULT:" + JSON.stringify(result));
process.exit(0);
}

main().catch(err => {
console.error("ERROR:" + err.message);
process.exit(2);
});
`;

fs.writeFileSync(scriptPath, script);

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
},
Comment on lines +133 to +138

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.

⚠️ Potential issue | 🟡 Minor

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.

});

// Should exit 0 and return null (no Brave key → skip)
expect(result.status).toBe(0);
expect(result.stdout).toContain("RESULT:null");

fs.rmSync(tmpDir, { recursive: true, force: true });
});
});