scripts/gemini-adapt-agents.js (2)

72-80: Redundant file read: content is read twice per file.

The file is read at line 74 (original) and again inside adaptFile at line 38. Consider passing the content to adaptFile to avoid redundant I/O.

♻️ Refactor to pass content and avoid double read

Update the function signature:

-function adaptFile(fp) {
-  const content = fs.readFileSync(fp, 'utf8');
+function adaptContent(content) {
   let result = content;

Update the call site:

 for (const f of files) {
   const fp = path.join(agentsDir, f);
   const original = fs.readFileSync(fp, 'utf8');
-  const adapted = adaptFile(fp);
+  const adapted = adaptContent(original);
   if (adapted !== original) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/gemini-adapt-agents.js` around lines 72 - 80, The loop reads each
file into `original` and `adaptFile(fp)` reads the same file again, causing
redundant I/O; change `adaptFile` to accept file content (e.g.,
`adaptFile(content, fp?)`) and update the call site in the for-loop to pass the
already-read `original` instead of the path, then have `adaptFile` operate on
the passed content (optionally keeping `fp` for metadata) and remove the
internal `fs.readFileSync` call—ensure `fs.writeFileSync(fp, adapted)` and the
`changed++` logic remain unchanged.

---

29-30: Minor inconsistency: WebSearch and WebFetch lack lowercase variants.

Other tools have both PascalCase and lowercase mappings (e.g., Read/read, Bash/bash), but WebSearch and WebFetch only have PascalCase. While existing agents appear to use PascalCase consistently, adding lowercase variants would maintain the defensive pattern.

🔧 Add lowercase variants for consistency

   WebSearch: 'google_web_search',
   WebFetch: 'web_fetch',
+  websearch: 'google_web_search',
+  webfetch: 'web_fetch',
 };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/gemini-adapt-agents.js` around lines 29 - 30, Add lowercase variants
for the two tool keys so mappings remain consistent: duplicate the existing
WebSearch and WebFetch entries by adding 'websearch': 'google_web_search' and
'webfetch': 'web_fetch' alongside the PascalCase keys (refer to the existing
WebSearch and WebFetch entries in the mapping). This preserves the defensive
pattern used for other tools (e.g., Read/read, Bash/bash) and ensures agents can
look up tools via lowercase keys.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/scripts/gemini-adapt-agents.test.js (1)

143-147: Avoid hardcoded missing path in CLI failure test.

/nonexistent/path/agents is brittle and not guaranteed absent on every environment. Build a guaranteed-missing path under a temp dir for deterministic behavior.

More deterministic missing-directory setup

   if (test('CLI errors on missing directory', () => {
-    const result = run(['/nonexistent/path/agents']);
-    assert.strictEqual(result.code, 1);
-    assert.ok(result.stderr.includes('Directory not found'));
+    const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gaa-missing-'));
+    const missing = path.join(base, 'does-not-exist');
+    try {
+      const result = run([missing]);
+      assert.strictEqual(result.code, 1);
+      assert.ok(result.stderr.includes('Directory not found'));
+    } finally {
+      fs.rmSync(base, { recursive: true, force: true });
+    }
   })) passed++; else failed++;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/scripts/gemini-adapt-agents.test.js` around lines 143 - 147, The test
"CLI errors on missing directory" is using a hardcoded path
'/nonexistent/path/agents' which may exist on some systems; modify the test to
generate a guaranteed-missing path under the system temp directory instead of a
hardcoded path: create a unique temp-based pathname (e.g., using os.tmpdir() + a
random/UUID component or by creating and removing a temp folder) and pass that
path to run(...) in the test, then assert result.code and stderr as before;
update the test declaration (the test('CLI errors on missing directory', ...))
to use this generated missingPath so the test is deterministic across
environments while still exercising run().

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/scripts/gemini-adapt-agents.test.js (2)

78-86: Consider adding test coverage for WebFetch tool mapping.

The TOOL_MAP in scripts/gemini-adapt-agents.js includes WebFetch: 'web_fetch' (per context snippet 1), but there's no dedicated test for this mapping. Adding a test would ensure complete coverage of all supported tool conversions.

Suggested addition (after line 81)

   })) passed++; else failed++;

+  if (test('maps WebFetch → web_fetch', () => {
+    const input = '---\ntools: [WebFetch]\n---\n';
+    assert.ok(adaptContent(input).includes('"web_fetch"'));
+  })) passed++; else failed++;
+
   if (test('converts mcp__server__tool → mcp_server_tool', () => {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/scripts/gemini-adapt-agents.test.js` around lines 78 - 86, Add a test
that verifies the WebFetch → web_fetch mapping by calling adaptContent with a
frontmatter entry using WebFetch and asserting the output includes the string
"web_fetch"; locate tests near the existing adaptContent checks (e.g., the tests
around 'maps WebSearch → google_web_search' and 'converts mcp__server__tool →
mcp_server_tool') and add a similar if (test(...)) passed++/else failed++ block
that uses adaptContent and assert.ok to confirm the conversion for the WebFetch
key present in TOOL_MAP in scripts/gemini-adapt-agents.js.

---

45-180: Test runner function exceeds 50-line guideline but is acceptable here.

At ~135 lines, runTests() exceeds the guideline for function length. However, for a sequential test runner with flat structure and no complex branching, this is a reasonable trade-off for readability. If the test suite grows significantly, consider grouping tests into separate functions (e.g., runUnitTests(), runCliTests()).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/scripts/gemini-adapt-agents.test.js` around lines 45 - 180, runTests()
is longer than the 50-line guideline; split it into smaller logical functions to
improve maintainability. Create two new functions (e.g., runUnitTests() and
runCliTests()) and move the flat sequence of unit test blocks (all
adaptContent/assert checks) into runUnitTests() and the CLI integration blocks
(calls to run(), tmpDir setups, fs assertions) into runCliTests(); then have
runTests() simply call runUnitTests(); runCliTests(); aggregate passed/failed
counts and keep existing process.exit behavior. Ensure you preserve shared
variables and helper calls (adaptContent, run, fs, path, os, assert) and
return/update the passed/failed counters from the new functions so runTests()
can log the final summary exactly as before.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/scripts/gemini-adapt-agents.test.js (2)

45-180: runTests is too large and repetitive—split into small grouped runners.

This function is difficult to scan/extend due to length and repeated pass/fail boilerplate. Consider extracting grouped test suites (runAdaptContentTests, runCliTests) and a tiny result accumulator helper.

♻️ Suggested refactor shape

-function runTests() {
-  console.log('\n=== Testing gemini-adapt-agents.js ===\n');
-  let passed = 0;
-  let failed = 0;
-  // ...many inline if (test(...)) passed++; else failed++;
-  console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`);
-  process.exit(failed > 0 ? 1 : 0);
-}
+function runCase(name, fn, stats) {
+  if (test(name, fn)) stats.passed++;
+  else stats.failed++;
+}
+
+function runAdaptContentTests(stats) {
+  runCase('maps Read → read_file', () => {
+    const input = '---\ntools: [Read]\n---\n';
+    assert.ok(adaptContent(input).includes('"read_file"'));
+  }, stats);
+  // ...other adaptContent cases
+}
+
+function runCliTests(stats) {
+  runCase('CLI errors on missing directory', () => {
+    // ...
+  }, stats);
+  // ...other CLI cases
+}
+
+function runTests() {
+  console.log('\n=== Testing gemini-adapt-agents.js ===\n');
+  const stats = { passed: 0, failed: 0 };
+  runAdaptContentTests(stats);
+  runCliTests(stats);
+  console.log(`\nResults: Passed: ${stats.passed}, Failed: ${stats.failed}`);
+  process.exit(stats.failed > 0 ? 1 : 0);
+}

As per coding guidelines, “Keep functions small (less than 50 lines)” and “Keep functions small (<50 lines) and files focused (<800 lines, typical 200-400 lines)”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/scripts/gemini-adapt-agents.test.js` around lines 45 - 180, The
runTests function is too large and repetitive; split it into smaller, focused
runners by extracting the adapt-content unit tests into a new
runAdaptContentTests function and the CLI tests into runCliTests, and add a tiny
result accumulator helper (e.g., TestResults or accumulateResult) to track
passed/failed counts; update runTests to call these helpers and print/exit based
on the combined accumulator. Locate the existing runTests, move the individual
if(test(...)) blocks that assert adaptContent behavior into runAdaptContentTests
(keeping their assertions intact and returning a TestResults object), move the
CLI integration blocks into runCliTests, implement a small helper used by both
to increment passed/failed, and ensure runTests only orchestrates calling the
two runners, printing combined results and exiting with the correct code.

---

143-147: Use a generated missing path instead of a hardcoded POSIX path.

Line 144 uses '/nonexistent/path/agents', which is brittle across environments. Build a definitely-missing path from a temp dir to keep the test platform-agnostic.

✅ Suggested change

-  if (test('CLI errors on missing directory', () => {
-    const result = run(['/nonexistent/path/agents']);
+  if (test('CLI errors on missing directory', () => {
+    const missingDir = path.join(
+      fs.mkdtempSync(path.join(os.tmpdir(), 'gaa-missing-')),
+      'agents-does-not-exist'
+    );
+    const result = run([missingDir]);
     assert.strictEqual(result.code, 1);
     assert.ok(result.stderr.includes('Directory not found'));
   })) passed++; else failed++;

Based on learnings, “Use Node.js scripts in the scripts directory for platform-agnostic helper tools that support Windows, macOS, and Linux.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/scripts/gemini-adapt-agents.test.js` around lines 143 - 147, The test
"CLI errors on missing directory" currently uses a hardcoded POSIX path
'/nonexistent/path/agents' which fails on non-Unix systems; replace that literal
with a platform-agnostic generated missing path using Node's os.tmpdir() (and
path.join) so the path is definitely absent on the current system before calling
run(). Update the test block that calls run(...) (and inspects
result.code/result.stderr) to construct the missing directory path from a temp
directory (ensure it does not exist) instead of the hardcoded string,
referencing the same test name and run() invocation.

tests/scripts/gemini-adapt-agents.test.js (1)

28-33: Use process.execPath instead of hardcoded 'node' for CLI invocation.

Hardcoding 'node' can be flaky in some CI/shell environments. Prefer the current runtime path for portability.

Suggested patch

-    const stdout = execFileSync('node', [SCRIPT, ...args], {
+    const stdout = execFileSync(process.execPath, [SCRIPT, ...args], {
       encoding: 'utf8',
       stdio: ['pipe', 'pipe', 'pipe'],
       timeout: 10000,
     });

Based on learnings: "Use Node.js scripts in the scripts/ directory for cross-platform utilities that support Windows, macOS, and Linux."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/scripts/gemini-adapt-agents.test.js` around lines 28 - 33, The test
helper function run uses execFileSync with a hardcoded 'node' which can break in
some CI/shell environments; update the call in run to invoke the current Node
binary via process.execPath instead of the string 'node' (leave the rest of the
execFileSync options and the SCRIPT and args parameters unchanged) so
execFileSync(SCRIPT invocation) uses process.execPath to improve
cross-platform/CI reliability.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes