Reduce unnecessary function callings to Gemini#135
Closed
masap wants to merge 6 commits intoreceptron:mainfrom
Closed
Reduce unnecessary function callings to Gemini#135masap wants to merge 6 commits intoreceptron:mainfrom
Gemini#135masap wants to merge 6 commits intoreceptron:mainfrom
Conversation
[1] https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/String/substr Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
Resolves receptron#123. According to official documentation [1], we should specify no more than 20 function callings to Gemini . [1] https://ai.google.dev/gemini-api/docs/function-calling?example=meeting#native-tools Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
📝 WalkthroughWalkthroughCentralized function-calling selection in Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server
participant GoogleGenAI
participant Gemini
participant Tools
Client->>Server: send generation request
Server->>Server: getFunctionCallingConfig(ai, config, params)
Server->>GoogleGenAI: instantiate with model
Server->>GoogleGenAI: generateContent(model, contents, config)
GoogleGenAI->>Gemini: request (with tool declarations / optional subset request)
Gemini-->>GoogleGenAI: response (content, selected tool indices / function calls)
GoogleGenAI-->>Server: content + toolCalls
Server->>Tools: extract toolCalls (ids via substring) and invoke tools
Tools-->>Server: tool results
Server-->>Client: aggregated response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
server/llm/providers/google.ts (3)
213-222: Theif (ret)check is always truthy.
getFunctionCallingConfigalways returns an object, neverundefinedornull. Consider checking for non-empty declarations instead, or remove the conditional.♻️ Optional: check for non-empty declarations
const ret = await getFunctionCallingConfig(ai, config, params); - if (ret) { + if (ret.functionDeclarations.length > 0) { config.tools = [{ functionDeclarations: ret.functionDeclarations }];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/llm/providers/google.ts` around lines 213 - 222, getFunctionCallingConfig(...) always returns an object so the current if (ret) is redundant; update the conditional to verify there are actual function declarations (e.g., check ret.functionDeclarations && ret.functionDeclarations.length > 0) before setting config.tools and config.toolConfig, or remove the if and always assign if you intend to apply an empty config; reference getFunctionCallingConfig, ret, config.tools, config.toolConfig and FunctionCallingConfigMode.ANY when making the change.
151-162: Consider the cost and latency of the additional API call.When tools exceed 20, this makes an extra LLM call to select tools before the actual generation. For high-traffic scenarios, consider:
- Caching selection results for similar conversation contexts
- Using a smaller/faster model for tool selection
- Deterministic filtering based on conversation keywords
This is acceptable for now but worth monitoring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/llm/providers/google.ts` around lines 151 - 162, The current tool-selection flow triggers an extra LLM call via ai.models.generateContent (using normalizeModelId and toGeminiMessages) when tools > 20, which can increase cost/latency; mitigate by adding a selection cache keyed by conversation context/signature to avoid repeated generateContent calls for similar contexts, optionally route selection to a configurable smaller/faster model (expose a separate selectionModel config used before calling normalizeModelId) or implement a deterministic pre-filter (keyword-based filter) to reduce candidates before invoking ai.models.generateContent; update the code paths around generateContent to consult the cache and selectionModel or run deterministic filtering first, and ensure cache invalidation/TTL is configurable.
164-166: Consider using structured logging instead ofconsole.log.Per coding guidelines, prefer the existing structured logging pattern and minimize use of
console.*in committed code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/llm/providers/google.ts` around lines 164 - 166, Replace the console.log call in server/llm/providers/google.ts with the project's structured logger (e.g., processLogger.info or logger.info) and emit the same message as both a human-readable string and structured fields: include MAX_FUNCTION_CALLINGS as maxFunctionCallings and the tools used (indices.map(i => tools[i].name)) as usedTools (array). Locate the console.log invocation that references MAX_FUNCTION_CALLINGS, indices, and tools and swap it for a structured log entry that passes the message and the two structured fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/llm/providers/google.ts`:
- Around line 141-143: The current construction of toolList builds JSON by
interpolating t.description directly, which can break when descriptions contain
quotes, backslashes or newlines; change the mapping to safely escape
descriptions by using JSON.stringify for the description (or better, build
actual objects { index: i, description: t.description } and call JSON.stringify
on the array) so that toolList is valid JSON; update the code around the
tools.map(...) that creates toolList to use JSON.stringify accordingly.
- Around line 163-166: The LLM response parsing is unsafe: when setting indices
from response.text you must wrap JSON.parse in a try/catch and validate the
parsed value is an array of valid integer indices within 0..tools.length-1 (and
optionally dedupe and limit to MAX_FUNCTION_CALLINGS); on parse error or invalid
items fall back to an empty array and log the error with context (use
console.error or the module logger) so the subsequent console.log/indices.map
won't access undefined tools entries; update the code around
response.text/indices and the console.log that maps indices to tools to use the
validated indices.
---
Nitpick comments:
In `@server/llm/providers/google.ts`:
- Around line 213-222: getFunctionCallingConfig(...) always returns an object so
the current if (ret) is redundant; update the conditional to verify there are
actual function declarations (e.g., check ret.functionDeclarations &&
ret.functionDeclarations.length > 0) before setting config.tools and
config.toolConfig, or remove the if and always assign if you intend to apply an
empty config; reference getFunctionCallingConfig, ret, config.tools,
config.toolConfig and FunctionCallingConfigMode.ANY when making the change.
- Around line 151-162: The current tool-selection flow triggers an extra LLM
call via ai.models.generateContent (using normalizeModelId and toGeminiMessages)
when tools > 20, which can increase cost/latency; mitigate by adding a selection
cache keyed by conversation context/signature to avoid repeated generateContent
calls for similar contexts, optionally route selection to a configurable
smaller/faster model (expose a separate selectionModel config used before
calling normalizeModelId) or implement a deterministic pre-filter (keyword-based
filter) to reduce candidates before invoking ai.models.generateContent; update
the code paths around generateContent to consult the cache and selectionModel or
run deterministic filtering first, and ensure cache invalidation/TTL is
configurable.
- Around line 164-166: Replace the console.log call in
server/llm/providers/google.ts with the project's structured logger (e.g.,
processLogger.info or logger.info) and emit the same message as both a
human-readable string and structured fields: include MAX_FUNCTION_CALLINGS as
maxFunctionCallings and the tools used (indices.map(i => tools[i].name)) as
usedTools (array). Locate the console.log invocation that references
MAX_FUNCTION_CALLINGS, indices, and tools and swap it for a structured log entry
that passes the message and the two structured fields.
server/llm/providers/google.ts
Outdated
Comment on lines
+163
to
+166
| indices = response.text === undefined ? [] : JSON.parse(response.text); | ||
| console.log( | ||
| `Gemini recommends using ${MAX_FUNCTION_CALLINGS} or fewer function callings. We use only ${indices.map((i) => tools[i].name)}.`, | ||
| ); |
There was a problem hiding this comment.
Add error handling for LLM response parsing and validate returned indices.
The LLM may return invalid JSON or out-of-bounds indices. Missing validation could cause runtime crashes or accessing undefined tool entries.
🛡️ Proposed fix with validation
- indices = response.text === undefined ? [] : JSON.parse(response.text);
- console.log(
- `Gemini recommends using ${MAX_FUNCTION_CALLINGS} or fewer function callings. We use only ${indices.map((i) => tools[i].name)}.`,
- );
+ try {
+ const parsed = response.text === undefined ? [] : JSON.parse(response.text);
+ // Validate indices: must be numbers within bounds, deduplicated, and limited to max
+ indices = [...new Set(parsed)]
+ .filter((i): i is number => typeof i === "number" && i >= 0 && i < tools.length)
+ .slice(0, MAX_FUNCTION_CALLINGS);
+ } catch {
+ // Fallback to first MAX_FUNCTION_CALLINGS tools if parsing fails
+ indices = tools.slice(0, MAX_FUNCTION_CALLINGS).map((_, i) => i);
+ }
+ console.log(
+ `Gemini recommends using ${MAX_FUNCTION_CALLINGS} or fewer function callings. We use only ${indices.map((i) => tools[i].name)}.`,
+ );📝 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
| indices = response.text === undefined ? [] : JSON.parse(response.text); | |
| console.log( | |
| `Gemini recommends using ${MAX_FUNCTION_CALLINGS} or fewer function callings. We use only ${indices.map((i) => tools[i].name)}.`, | |
| ); | |
| try { | |
| const parsed = response.text === undefined ? [] : JSON.parse(response.text); | |
| // Validate indices: must be numbers within bounds, deduplicated, and limited to max | |
| indices = [...new Set(parsed)] | |
| .filter((i): i is number => typeof i === "number" && i >= 0 && i < tools.length) | |
| .slice(0, MAX_FUNCTION_CALLINGS); | |
| } catch { | |
| // Fallback to first MAX_FUNCTION_CALLINGS tools if parsing fails | |
| indices = tools.slice(0, MAX_FUNCTION_CALLINGS).map((_, i) => i); | |
| } | |
| console.log( | |
| `Gemini recommends using ${MAX_FUNCTION_CALLINGS} or fewer function callings. We use only ${indices.map((i) => tools[i].name)}.`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/llm/providers/google.ts` around lines 163 - 166, The LLM response
parsing is unsafe: when setting indices from response.text you must wrap
JSON.parse in a try/catch and validate the parsed value is an array of valid
integer indices within 0..tools.length-1 (and optionally dedupe and limit to
MAX_FUNCTION_CALLINGS); on parse error or invalid items fall back to an empty
array and log the error with context (use console.error or the module logger) so
the subsequent console.log/indices.map won't access undefined tools entries;
update the code around response.text/indices and the console.log that maps
indices to tools to use the validated indices.
Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
…ices Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/llm/providers/google.ts (1)
166-175:⚠️ Potential issue | 🟠 MajorRequire integer index validation before dereferencing tools.
At Line 172-174, decimals (e.g.
1.5) pass the filter. That can producetools[i] === undefined, then break at Line 181/189/193 when accessing.name/.description/.parameters.🔒 Proposed hardening
try { - const parsed = - response.text === undefined ? [] : JSON.parse(response.text); + const parsed = + response.text === undefined ? [] : JSON.parse(response.text); + if (!Array.isArray(parsed)) { + throw new Error("Function-calling selector returned non-array JSON"); + } // Validate indices: must be numbers within bounds, deduplicated, and limited to max indices = [...new Set(parsed)] .filter( (i): i is number => - typeof i === "number" && i >= 0 && i < tools.length, + Number.isInteger(i) && i >= 0 && i < tools.length, ) .slice(0, MAX_FUNCTION_CALLINGS); } catch { // Fallback to first MAX_FUNCTION_CALLINGS tools if parsing fails indices = tools.slice(0, MAX_FUNCTION_CALLINGS).map((_, i) => i); }#!/bin/bash # Verify the current predicate and dereference sites in this file. rg -nP 'typeof i === "number" && i >= 0 && i < tools\.length|tools\[i\]\.(name|description|parameters)' server/llm/providers/google.tsAlso applies to: 181-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/llm/providers/google.ts` around lines 166 - 175, Parsed indices can include non-integer numbers (e.g. 1.5) which pass the current typeof check and later cause tools[i] to be undefined; update the filter predicate used when creating indices (the expression that deduplicates and filters parsed into indices for tools) to require integer indices (use Number.isInteger(i) or equivalent) in addition to the existing bounds check and keep the slice(0, MAX_FUNCTION_CALLINGS) behavior so only valid integer indices within [0, tools.length) are included before any dereference of tools[i].
🧹 Nitpick comments (1)
server/llm/providers/google.ts (1)
180-182: Replaceconsole.logwith the project's structured logger.Import
loggerfrom../utils/loggerand uselogger.info()to align with the established structured logging pattern used throughout the server codebase, per the coding guideline: "Prefer the existing structured logging pattern and minimize use ofconsole.*in committed code".logger.info( `Gemini recommends using ${MAX_FUNCTION_CALLINGS} or fewer function callings. We use only ${indices.map((i) => tools[i].name)}.`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/llm/providers/google.ts` around lines 180 - 182, Replace the console.log call in the Gemini recommendation block with the project's structured logger: import logger from ../utils/logger and change the call to logger.info(...) using the same message string that references MAX_FUNCTION_CALLINGS and the indices.map((i) => tools[i].name) expression; update the file-level imports and ensure the replacement occurs where the current console.log invocation exists in server/llm/providers/google.ts so the code uses logger.info instead of console.log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/llm/providers/google.ts`:
- Around line 154-165: The pre-filter selector call using
ai.models.generateContent (called with model: normalizeModelId(model) and
contents: toGeminiMessages(message)) must not abort the whole generation on
failure; wrap that generateContent call in a try/catch and on any error
(network/4xx/5xx) log the error and fallback to selecting the first 20 tools
instead of throwing; apply the same pattern to the other selector invocation
around the code referenced at the later block (the second generateContent call
at the 176-179 area), ensuring both use the same try/catch fallback to first 20
tools.
- Around line 141-146: The tool-list JSON entries are malformed due to an extra
closing brace in the string construction inside the tools.map used to build
toolList; update the mapping in the toolList construction (the tools.map
callback that references t and i and assigns to toolList) to remove the stray
"}" so each entry is valid JSON (e.g. produce
{"index":<i>,"description":<JSON.stringify(t.description)>} for each tool) and
keep the join logic unchanged.
---
Duplicate comments:
In `@server/llm/providers/google.ts`:
- Around line 166-175: Parsed indices can include non-integer numbers (e.g. 1.5)
which pass the current typeof check and later cause tools[i] to be undefined;
update the filter predicate used when creating indices (the expression that
deduplicates and filters parsed into indices for tools) to require integer
indices (use Number.isInteger(i) or equivalent) in addition to the existing
bounds check and keep the slice(0, MAX_FUNCTION_CALLINGS) behavior so only valid
integer indices within [0, tools.length) are included before any dereference of
tools[i].
---
Nitpick comments:
In `@server/llm/providers/google.ts`:
- Around line 180-182: Replace the console.log call in the Gemini recommendation
block with the project's structured logger: import logger from ../utils/logger
and change the call to logger.info(...) using the same message string that
references MAX_FUNCTION_CALLINGS and the indices.map((i) => tools[i].name)
expression; update the file-level imports and ensure the replacement occurs
where the current console.log invocation exists in
server/llm/providers/google.ts so the code uses logger.info instead of
console.log.
server/llm/providers/google.ts
Outdated
Comment on lines
+154
to
+165
| const response = await ai.models.generateContent({ | ||
| model: normalizeModelId(model), | ||
| contents: toGeminiMessages(message), | ||
| config: { | ||
| ...config, | ||
| responseMimeType: "application/json", | ||
| responseJsonSchema: { | ||
| type: "array", | ||
| items: { type: "integer" }, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Handle selector API failures with graceful fallback.
If the selector generateContent call fails (network/400/500), the whole generation aborts before the main request. For this pre-filter step, fallback to first 20 tools instead of failing hard.
🛡️ Suggested resilience pattern
- const response = await ai.models.generateContent({
- model: normalizeModelId(model),
- contents: toGeminiMessages(message),
- config: {
- ...config,
- responseMimeType: "application/json",
- responseJsonSchema: {
- type: "array",
- items: { type: "integer" },
- },
- },
- });
- try {
+ try {
+ const response = await ai.models.generateContent({
+ model: normalizeModelId(model),
+ contents: toGeminiMessages(message),
+ config: {
+ ...config,
+ responseMimeType: "application/json",
+ responseJsonSchema: {
+ type: "array",
+ items: { type: "integer" },
+ },
+ },
+ });
const parsed =
response.text === undefined ? [] : JSON.parse(response.text);
@@
- } catch {
- // Fallback to first MAX_FUNCTION_CALLINGS tools if parsing fails
+ } catch {
+ // Fallback if selector call or parsing fails
indices = tools.slice(0, MAX_FUNCTION_CALLINGS).map((_, i) => i);
}Also applies to: 176-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/llm/providers/google.ts` around lines 154 - 165, The pre-filter
selector call using ai.models.generateContent (called with model:
normalizeModelId(model) and contents: toGeminiMessages(message)) must not abort
the whole generation on failure; wrap that generateContent call in a try/catch
and on any error (network/4xx/5xx) log the error and fallback to selecting the
first 20 tools instead of throwing; apply the same pattern to the other selector
invocation around the code referenced at the later block (the second
generateContent call at the 176-179 area), ensuring both use the same try/catch
fallback to first 20 tools.
Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/llm/providers/google.ts (1)
170-175:⚠️ Potential issue | 🟠 MajorEnforce integer indices before indexing
tools.
typeof i === "number"also accepts floats. A value like1.5passes this filter, thentools[i]isundefinedand can fail later at Line 190.🔧 Proposed fix
- indices = [...new Set(parsed)] - .filter( - (i): i is number => - typeof i === "number" && i >= 0 && i < tools.length, - ) - .slice(0, MAX_FUNCTION_CALLINGS); + indices = [...new Set(parsed)] + .filter( + (i): i is number => + Number.isInteger(i) && i >= 0 && i < tools.length, + ) + .slice(0, MAX_FUNCTION_CALLINGS);#!/bin/bash # Verify current selector guard accepts all numbers (including floats) rg -nP 'typeof i === "number"\s*&&\s*i >= 0\s*&&\s*i < tools.length' server/llm/providers/google.ts # Verify downstream dereference of tools[i] rg -nP 'tools\[i\]\.(name|description|parameters)' server/llm/providers/google.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/llm/providers/google.ts` around lines 170 - 175, The indices filter currently allows non-integer numbers (e.g., 1.5) because it only checks typeof i === "number"; update the validation in the block that assigns indices (the parsed -> indices dedupe/filter logic) to also require integer indices (use Number.isInteger(i)) and keep existing bounds and type checks so only integers 0 <= i < tools.length survive before any dereference of tools[i]; ensure you modify the filter predicate that currently reads (i): i is number => typeof i === "number" && i >= 0 && i < tools.length to include a Number.isInteger(i) guard.
🧹 Nitpick comments (1)
server/llm/providers/google.ts (1)
181-183: Use the structured logger instead ofconsole.log.Please route this through the project logger to keep server-side logging consistent.
As per coding guidelines: "Prefer the existing structured logging pattern and minimize use of
console.*in committed code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/llm/providers/google.ts` around lines 181 - 183, Replace the console.log call that prints Gemini recommendation and used tool names with the project's structured logger (e.g., logger.info or serverLogger.info). Locate the statement that references MAX_FUNCTION_CALLINGS and indices.map((i) => tools[i].name) and emit a structured log containing a clear message plus fields like { maxFunctionCallings: MAX_FUNCTION_CALLINGS, usedTools: indices.map(i => tools[i].name) }. Also ensure the appropriate logger is imported/available in this module and remove the console.log usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/llm/providers/google.ts`:
- Around line 170-175: The indices filter currently allows non-integer numbers
(e.g., 1.5) because it only checks typeof i === "number"; update the validation
in the block that assigns indices (the parsed -> indices dedupe/filter logic) to
also require integer indices (use Number.isInteger(i)) and keep existing bounds
and type checks so only integers 0 <= i < tools.length survive before any
dereference of tools[i]; ensure you modify the filter predicate that currently
reads (i): i is number => typeof i === "number" && i >= 0 && i < tools.length to
include a Number.isInteger(i) guard.
---
Nitpick comments:
In `@server/llm/providers/google.ts`:
- Around line 181-183: Replace the console.log call that prints Gemini
recommendation and used tool names with the project's structured logger (e.g.,
logger.info or serverLogger.info). Locate the statement that references
MAX_FUNCTION_CALLINGS and indices.map((i) => tools[i].name) and emit a
structured log containing a clear message plus fields like {
maxFunctionCallings: MAX_FUNCTION_CALLINGS, usedTools: indices.map(i =>
tools[i].name) }. Also ensure the appropriate logger is imported/available in
this module and remove the console.log usage.
snakajima
approved these changes
Mar 16, 2026
Contributor
|
Addressing this issue with #144 |
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.
Resolves #123.
According to official documentation [1], we should specify no more than 20 function callings to Gemini .
[1] https://ai.google.dev/gemini-api/docs/function-calling?example=meeting#native-tools
Summary by CodeRabbit