feat: add Gemini function-calling framework with safe tool registry - #37
feat: add Gemini function-calling framework with safe tool registry#37GEEKYFOCUS wants to merge 1 commit into
Conversation
Implements a function-calling (tool-calling) system for the /chat endpoint, allowing the model to invoke platform capabilities via a pydantic-validated tool registry. Key changes: - New tools/ package with Tool, ToolRegistry, schema bridging - Pydantic v2 model_json_schema() -> Gemini FunctionDeclaration - Execution loop in /chat (max 4 rounds, per-tool timeouts via ThreadPoolExecutor) - ChatResponse.tool_calls optional field for frontend transparency - Three initial tools: calculate_zakat, get_stellar_info, search_courses - Stellar zakat logic refactored into tools/handlers.py for sharing between REST endpoint and tool - BACKEND_API_URL env var for course-search backend - 14 offline tests with fake model client (scripted function_call parts) - Updated CI to lint tools/ and run tool registry tests Closes Deen-Bridge#25
WalkthroughAdds a Gemini function-calling framework with a validated tool registry, Stellar and course tools, bounded chat execution, recorded tool calls, shared zakat computation, comprehensive tests, CI checks, and updated API documentation. ChangesGemini tool-calling framework
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Chat
participant Gemini
participant ToolRegistry
participant ToolHandler
Client->>Chat: Submit prompt
Chat->>Gemini: Send prompt with tool declarations
Gemini-->>Chat: Return function call
Chat->>ToolRegistry: Resolve registered tool
ToolRegistry->>ToolHandler: Validate and execute arguments
ToolHandler-->>Chat: Return structured result
Chat->>Gemini: Send function response
Gemini-->>Chat: Return final text
Chat-->>Client: Return response and tool_calls
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/ci.ymlTraceback (most recent call last): 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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 31: Update the flake8 command in the CI workflow to include the changed
test files tests/test_tools.py and tests/conftest.py, or lint the tests/
directory comprehensively. Preserve the existing max-line-length and ignore
settings so these files are checked under the same CI rules.
In `@main.py`:
- Around line 185-186: Guard the response handling around
candidate.content.parts in the chat flow before indexing candidates[0] or
parts[0]. If either collection is empty, return a clear user-facing error
without exposing raw exception details; preserve the existing processing path
when both contain an item. Apply the same protection at the corresponding later
access around the alternate response handling.
- Around line 166-229: Offload the synchronous generate flow from the async chat
handler by replacing direct calls to generate, including the
SafetyPipeline._complete path and non-safety branch, with await
asyncio.to_thread(generate, safety_prompt) or the project’s threadpool
equivalent. Preserve prompt selection and response handling while ensuring the
blocking _run_with_tools loop never executes on the event-loop thread.
In `@stellar.py`:
- Around line 87-95: Update the stellar_info endpoint’s "network" field to
return the configured Stellar network name via the existing STELLAR_NETWORK
value, matching tools/handlers.py::_get_stellar_info, while leaving the separate
"horizon" field mapped to _horizon_url().
In `@tests/test_tools.py`:
- Around line 271-303: Update test_loop_bounded and its fake chat responses to
provide more than MAX_TOOL_ROUNDS function_call responses, then assert
_run_with_tools still returns after exactly MAX_TOOL_ROUNDS tool calls and
produces the forced final text. Keep the existing registry setup and cleanup
unchanged.
In `@tools/handlers.py`:
- Around line 80-91: Update CalculateZakatArgs.nisab_usd to enforce the same
strictly-positive validation as stellar.py’s ZakatRequest, rejecting negative
and zero overrides at the tool boundary. In _calculate_zakat, distinguish an
omitted value from an explicit value by checking for None rather than
truthiness, while preserving DEFAULT_NISAB_USD only when nisab_usd is absent.
In `@tools/registry.py`:
- Around line 108-112: Update the exception handlers around tool argument
validation and execution to return only generic, sanitized error messages
without interpolating exc or raw validation details into the tool result. Log
the caught exception server-side with appropriate context, while preserving the
existing error-result flow and tool name context.
- Around line 106-134: The fixed four-worker _EXECUTOR allows timed-out handlers
to continue running and exhaust the shared pool. Update the execution design
around run_tool_handler and _EXECUTOR so timed-out tool calls cannot
indefinitely starve other requests, using an actually interruptible mechanism or
an appropriate bounded-concurrency/watchdog strategy; preserve argument
validation, timeout error reporting, and result logging.
🪄 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: Pro Plus
Run ID: 1775cf92-8a48-40aa-806d-6bff779170b4
📒 Files selected for processing (9)
.github/workflows/ci.ymlREADME.mdmain.pystellar.pytests/conftest.pytests/test_tools.pytools/__init__.pytools/handlers.pytools/registry.py
|
|
||
| - name: Run linting | ||
| run: flake8 main.py stellar.py safety tests/redteam study.py --max-line-length=120 --ignore=E501,W503 | ||
| run: flake8 main.py stellar.py safety tests/redteam study.py tools/ --max-line-length=120 --ignore=E501,W503 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Lint the new tool-test files too.
tests/test_tools.py and tests/conftest.py are changed Python files but are absent from the flake8 target, so their style or Pyflakes failures can merge unnoticed. Add both paths (or lint tests/ comprehensively).
Suggested CI adjustment
- run: flake8 main.py stellar.py safety tests/redteam study.py tools/ --max-line-length=120 --ignore=E501,W503
+ run: flake8 main.py stellar.py safety tests/redteam tests/test_tools.py tests/conftest.py study.py tools/ --max-line-length=120 --ignore=E501,W503As per path instructions, “CI enforces flake8, so style violations fail the build.”
📝 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.
| run: flake8 main.py stellar.py safety tests/redteam study.py tools/ --max-line-length=120 --ignore=E501,W503 | |
| run: flake8 main.py stellar.py safety tests/redteam tests/test_tools.py tests/conftest.py study.py tools/ --max-line-length=120 --ignore=E501,W503 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 31, Update the flake8 command in the CI
workflow to include the changed test files tests/test_tools.py and
tests/conftest.py, or lint the tests/ directory comprehensively. Preserve the
existing max-line-length and ignore settings so these files are checked under
the same CI rules.
Source: Path instructions
| def _run_with_tools(chat_session, prompt: str) -> tuple[str, list[ToolCallRecord]]: | ||
| """Run a prompt through the model with function-calling loop. | ||
|
|
||
| Returns (final_text, tool_calls_list). Sync — the tool handlers use | ||
| a thread-pool executor for per-tool timeouts. | ||
| """ | ||
| tool_calls: list[ToolCallRecord] = [] | ||
| content: Any = prompt | ||
|
|
||
| for _round in range(MAX_TOOL_ROUNDS): | ||
| response = chat_session.send_message( | ||
| content, | ||
| generation_config={ | ||
| "temperature": 0.7, | ||
| "top_p": 0.8, | ||
| "top_k": 40, | ||
| "max_output_tokens": 2048, | ||
| }, | ||
| ) | ||
| candidate = response.candidates[0] | ||
| part = candidate.content.parts[0] | ||
|
|
||
| if part.text: | ||
| return part.text, tool_calls | ||
|
|
||
| if part.function_call: | ||
| fc = part.function_call | ||
| args_dict = dict(fc.args) if fc.args else {} | ||
|
|
||
| tool = tool_registry.get(fc.name) | ||
| if tool is None: | ||
| result = {"error": f"Unknown tool: {fc.name}"} | ||
| else: | ||
| result = run_tool_handler(tool, args_dict) | ||
|
|
||
| tool_calls.append(ToolCallRecord( | ||
| tool_name=fc.name, | ||
| args=args_dict, | ||
| result=json.dumps(result), | ||
| )) | ||
|
|
||
| content = protos.Content( | ||
| parts=[protos.Part( | ||
| function_response=protos.FunctionResponse( | ||
| name=fc.name, | ||
| response=result, | ||
| ) | ||
| )], | ||
| role="user", | ||
| ) | ||
|
|
||
| # Exhausted rounds — force a final text answer | ||
| response = chat_session.send_message( | ||
| "Please provide your best final answer based on the information available.", | ||
| generation_config={ | ||
| "temperature": 0.7, | ||
| "top_p": 0.8, | ||
| "top_k": 40, | ||
| "max_output_tokens": 2048, | ||
| }, | ||
| ) | ||
| final_text = response.candidates[0].content.parts[0].text or "" | ||
| return final_text, tool_calls | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Fully synchronous Gemini + tool-calling loop runs on the event-loop thread.
generate() (and the _run_with_tools loop it now drives) performs up to MAX_TOOL_ROUNDS sequential blocking chat_session.send_message calls, each potentially followed by a blocking tool wait (up to timeout_seconds, e.g. 15s for zakat), plus one final forced-answer call — all inside async def chat(...) without ever being awaited or offloaded (SafetyPipeline._complete calls generator(...) synchronously; the non-safety branch calls generate(...) directly too). In the worst case this can block the event loop for a large multiple of a single Gemini round trip, stalling every other concurrent request on that worker.
Wrap the blocking generate() call in await asyncio.to_thread(generate, safety_prompt) (or FastAPI's run_in_threadpool) so the event loop stays responsive while the tool-calling loop runs.
As per path instructions, "Flag blocking calls inside async endpoints (network calls should be awaited or offloaded)" for this FastAPI service.
Also applies to: 240-274
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 203-203: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 166 - 229, Offload the synchronous generate flow from
the async chat handler by replacing direct calls to generate, including the
SafetyPipeline._complete path and non-safety branch, with await
asyncio.to_thread(generate, safety_prompt) or the project’s threadpool
equivalent. Preserve prompt selection and response handling while ensuring the
blocking _run_with_tools loop never executes on the event-loop thread.
Source: Path instructions
| candidate = response.candidates[0] | ||
| part = candidate.content.parts[0] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded indexing into candidates[0]/parts[0].
If the model response has no candidates (e.g. safety-blocked) or a candidate with no parts, this raises IndexError, which is caught only by the outermost bare except Exception in /chat (line 325) and surfaced to the client as a raw error string. Add explicit guards so this fails with a clear, non-leaking message.
Also applies to: 227-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 185 - 186, Guard the response handling around
candidate.content.parts in the chat flow before indexing candidates[0] or
parts[0]. If either collection is empty, return a clear user-facing error
without exposing raw exception details; preserve the existing processing path
when both contain an item. Apply the same protection at the corresponding later
access around the alternate response handling.
| @router.get("/stellar/info") | ||
| async def stellar_info(): | ||
| """Public configuration of this service's Stellar integration.""" | ||
| return { | ||
| "network": STELLAR_NETWORK, | ||
| "horizon": horizon_url(), | ||
| "usdc_issuer": usdc_issuer(), | ||
| "network": _horizon_url(), | ||
| "horizon": _horizon_url(), | ||
| "usdc_issuer": _usdc_issuer(), | ||
| "features": ["zakat"], | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
/stellar/info "network" field returns the Horizon URL instead of the network name.
"network": _horizon_url() duplicates the "horizon" field's value. tools/handlers.py::_get_stellar_info (the sibling implementation) correctly returns STELLAR_NETWORK (e.g. "testnet") for this field — this endpoint should match.
🐛 Proposed fix
from tools.handlers import (
DISCLAIMER,
+ STELLAR_NETWORK,
_calculate_zakat,
_horizon_url,
_usdc_issuer,
) return {
- "network": _horizon_url(),
+ "network": STELLAR_NETWORK,
"horizon": _horizon_url(),
"usdc_issuer": _usdc_issuer(),
"features": ["zakat"],
}📝 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.
| @router.get("/stellar/info") | |
| async def stellar_info(): | |
| """Public configuration of this service's Stellar integration.""" | |
| return { | |
| "network": STELLAR_NETWORK, | |
| "horizon": horizon_url(), | |
| "usdc_issuer": usdc_issuer(), | |
| "network": _horizon_url(), | |
| "horizon": _horizon_url(), | |
| "usdc_issuer": _usdc_issuer(), | |
| "features": ["zakat"], | |
| } | |
| `@router.get`("/stellar/info") | |
| async def stellar_info(): | |
| """Public configuration of this service's Stellar integration.""" | |
| return { | |
| "network": STELLAR_NETWORK, | |
| "horizon": _horizon_url(), | |
| "usdc_issuer": _usdc_issuer(), | |
| "features": ["zakat"], | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stellar.py` around lines 87 - 95, Update the stellar_info endpoint’s
"network" field to return the configured Stellar network name via the existing
STELLAR_NETWORK value, matching tools/handlers.py::_get_stellar_info, while
leaving the separate "horizon" field mapped to _horizon_url().
| @patch("tools.registry.run_tool_handler", return_value={"ok": True}) | ||
| def test_loop_bounded(mock_handler): | ||
| """Model that keeps returning function_calls cannot loop forever.""" | ||
| # Script MAX_TOOL_ROUNDS function_call responses — after that the forced | ||
| # text message will get the final_text from the mock. | ||
| responses = [{"name": "dummy", "args": {"name": "x", "count": 1}} for _ in range(MAX_TOOL_ROUNDS)] | ||
| chat = _make_fake_chat(responses, "Final after loop") | ||
| from main import _run_with_tools | ||
|
|
||
| registry = ToolRegistry() | ||
| tool = Tool( | ||
| name="dummy", | ||
| description="", | ||
| args_schema=SampleArgs, | ||
| handler=lambda **kw: {}, | ||
| timeout_seconds=5, | ||
| ) | ||
| registry.register(tool) | ||
| import main as main_module | ||
| original_reg = main_module.tool_registry | ||
| main_module.tool_registry = registry | ||
| original_decls = main_module.tool_declarations | ||
| main_module.tool_declarations = registry.declarations() | ||
|
|
||
| try: | ||
| text, calls = _run_with_tools(chat, "test") | ||
| # Should have MAX_TOOL_ROUNDS calls, then forced text | ||
| assert len(calls) == MAX_TOOL_ROUNDS | ||
| assert text == "Final after loop" | ||
| finally: | ||
| main_module.tool_registry = original_reg | ||
| main_module.tool_declarations = original_decls | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the bound test exceed the allowed call count.
The fake returns text immediately after exactly MAX_TOOL_ROUNDS calls, so an off-by-one or unbounded loop can still pass. Script at least one additional function call (or an endless-call fake) and assert execution stops at MAX_TOOL_ROUNDS.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_tools.py` around lines 271 - 303, Update test_loop_bounded and its
fake chat responses to provide more than MAX_TOOL_ROUNDS function_call
responses, then assert _run_with_tools still returns after exactly
MAX_TOOL_ROUNDS tool calls and produces the forced final text. Keep the existing
registry setup and cleanup unchanged.
| class CalculateZakatArgs(BaseModel): | ||
| public_key: str = Field(..., description="Stellar account public key (G...)") | ||
| nisab_usd: Optional[float] = Field(None, description="Override nisab threshold in USD") | ||
|
|
||
|
|
||
| def _calculate_zakat(public_key: str, nisab_usd: Optional[float] = None) -> dict: | ||
| if not StrKey.is_valid_ed25519_public_key(public_key): | ||
| return {"error": "Invalid Stellar public key. Expected a 56-character key starting with G."} | ||
|
|
||
| logger.info("Zakat lookup for %s on %s", public_key[:8], STELLAR_NETWORK) | ||
| balance = _fetch_usdc_balance(public_key) | ||
| nisab = Decimal(str(nisab_usd)) if nisab_usd else DEFAULT_NISAB_USD |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
nisab_usd validation gap vs. the REST endpoint, plus a falsy-zero bug.
CalculateZakatArgs.nisab_usd is missing the gt=0 constraint that stellar.py's ZakatRequest.nisab_usd enforces on the same shared computation. Since this tool is directly reachable by the model, a negative nisab_usd sails through validation and then balance >= nisab in _calculate_zakat is trivially true, producing an incorrect zakat verdict. Separately, if nisab_usd else DEFAULT_NISAB_USD treats an explicit 0 the same as "not provided."
As per path instructions, "missing Pydantic validation on request bodies" should be flagged for this FastAPI service's **/*.py files.
🛡️ Proposed fix
class CalculateZakatArgs(BaseModel):
public_key: str = Field(..., description="Stellar account public key (G...)")
- nisab_usd: Optional[float] = Field(None, description="Override nisab threshold in USD")
+ nisab_usd: Optional[float] = Field(None, gt=0, description="Override nisab threshold in USD")- nisab = Decimal(str(nisab_usd)) if nisab_usd else DEFAULT_NISAB_USD
+ nisab = Decimal(str(nisab_usd)) if nisab_usd is not None else DEFAULT_NISAB_USD📝 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.
| class CalculateZakatArgs(BaseModel): | |
| public_key: str = Field(..., description="Stellar account public key (G...)") | |
| nisab_usd: Optional[float] = Field(None, description="Override nisab threshold in USD") | |
| def _calculate_zakat(public_key: str, nisab_usd: Optional[float] = None) -> dict: | |
| if not StrKey.is_valid_ed25519_public_key(public_key): | |
| return {"error": "Invalid Stellar public key. Expected a 56-character key starting with G."} | |
| logger.info("Zakat lookup for %s on %s", public_key[:8], STELLAR_NETWORK) | |
| balance = _fetch_usdc_balance(public_key) | |
| nisab = Decimal(str(nisab_usd)) if nisab_usd else DEFAULT_NISAB_USD | |
| class CalculateZakatArgs(BaseModel): | |
| public_key: str = Field(..., description="Stellar account public key (G...)") | |
| nisab_usd: Optional[float] = Field(None, gt=0, description="Override nisab threshold in USD") | |
| def _calculate_zakat(public_key: str, nisab_usd: Optional[float] = None) -> dict: | |
| if not StrKey.is_valid_ed25519_public_key(public_key): | |
| return {"error": "Invalid Stellar public key. Expected a 56-character key starting with G."} | |
| logger.info("Zakat lookup for %s on %s", public_key[:8], STELLAR_NETWORK) | |
| balance = _fetch_usdc_balance(public_key) | |
| nisab = Decimal(str(nisab_usd)) if nisab_usd is not None else DEFAULT_NISAB_USD |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/handlers.py` around lines 80 - 91, Update CalculateZakatArgs.nisab_usd
to enforce the same strictly-positive validation as stellar.py’s ZakatRequest,
rejecting negative and zero overrides at the tool boundary. In _calculate_zakat,
distinguish an omitted value from an explicit value by checking for None rather
than truthiness, while preserving DEFAULT_NISAB_USD only when nisab_usd is
absent.
Source: Path instructions
| def run_tool_handler(tool: Tool, args: dict[str, Any]) -> dict[str, Any]: | ||
| """Validate args through pydantic and execute the handler with timeout.""" | ||
| try: | ||
| validated = tool.args_schema.model_validate(args) | ||
| validated_args = validated.model_dump(exclude_unset=True) | ||
| except Exception as exc: | ||
| return {"error": f"Argument validation failed for '{tool.name}': {exc}"} | ||
|
|
||
| started = time.perf_counter() | ||
| logger.info("Tool call: %s args=%s", tool.name, _truncate(validated_args, 200)) | ||
|
|
||
| try: | ||
| future = _EXECUTOR.submit(tool.handler, **validated_args) | ||
| result = future.result(timeout=tool.timeout_seconds) | ||
| except FutureTimeout: | ||
| result = {"error": f"Tool '{tool.name}' timed out after {tool.timeout_seconds}s"} | ||
| logger.warning("Tool timeout: %s (%ss)", tool.name, tool.timeout_seconds) | ||
| except Exception as exc: | ||
| result = {"error": f"Tool '{tool.name}' error: {exc}"} | ||
| logger.error("Tool error: %s — %s", tool.name, exc) | ||
|
|
||
| duration = time.perf_counter() - started | ||
| logger.info( | ||
| "Tool result: %s duration=%.2fs outcome=%s", | ||
| tool.name, | ||
| duration, | ||
| "error" if "error" in result else "ok", | ||
| ) | ||
| return result |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Timed-out tool calls don't free the worker slot.
future.result(timeout=tool.timeout_seconds) only stops the caller from waiting; the submitted thread keeps executing in the background (concurrent.futures cannot cancel running work). _EXECUTOR is a process-wide pool of only 4 workers shared by every concurrent chat request. A few slow/hanging Horizon or backend calls can occupy all 4 workers well past their nominal timeout, starving every other user's tool calls until the underlying requests finally resolve.
Consider bounding the blast radius, e.g., size the pool relative to expected concurrency, add a watchdog/metric on executor queue depth, or move to a mechanism that can actually interrupt slow I/O (subprocess/asyncio with true cancellation).
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 111-111: Do not catch blind exception: Exception
(BLE001)
[warning] 123-123: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/registry.py` around lines 106 - 134, The fixed four-worker _EXECUTOR
allows timed-out handlers to continue running and exhaust the shared pool.
Update the execution design around run_tool_handler and _EXECUTOR so timed-out
tool calls cannot indefinitely starve other requests, using an actually
interruptible mechanism or an appropriate bounded-concurrency/watchdog strategy;
preserve argument validation, timeout error reporting, and result logging.
| try: | ||
| validated = tool.args_schema.model_validate(args) | ||
| validated_args = validated.model_dump(exclude_unset=True) | ||
| except Exception as exc: | ||
| return {"error": f"Argument validation failed for '{tool.name}': {exc}"} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Broad excepts surface raw exception text to the model/client.
Both catches return str(exc)/validation-error text directly as the tool result, which flows into ToolCallRecord.result and is returned via ChatResponse.tool_calls to API consumers. Prefer a generic, sanitized error message and log the real exception server-side.
As per path instructions, "bare excepts that leak stack traces to clients" should be flagged for **/*.py files in this FastAPI service.
🛡️ Proposed fix
except Exception as exc:
- return {"error": f"Argument validation failed for '{tool.name}': {exc}"}
+ logger.warning("Validation failed for tool '%s': %s", tool.name, exc)
+ return {"error": f"Argument validation failed for '{tool.name}'."} except Exception as exc:
- result = {"error": f"Tool '{tool.name}' error: {exc}"}
- logger.error("Tool error: %s — %s", tool.name, exc)
+ result = {"error": f"Tool '{tool.name}' failed to execute."}
+ logger.error("Tool error: %s — %s", tool.name, exc)Also applies to: 117-125
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 111-111: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/registry.py` around lines 108 - 112, Update the exception handlers
around tool argument validation and execution to return only generic, sanitized
error messages without interpolating exc or raw validation details into the tool
result. Log the caught exception server-side with appropriate context, while
preserving the existing error-result flow and tool name context.
Source: Path instructions
|
@GEEKYFOCUS fix conflict please |
3 participants
Overview
This PR adds a Gemini function-calling (tool-calling) framework to the
/chatendpoint so the AI assistant can compute zakat, fetch Stellar network info, and search courses — all through pydantic-validated, timeout-enforced tool handlers.Related Issue
Closes #25
Verification Results
flake8 main.py stellar.py tools/ tests/test_tools.py— lint ✅ passedpython -m compileall main.py stellar.py tools/— syntax ✅ passedpytest tests/test_tools.py -v— 14/14 ✅ passedChanges
⚙️ Tool Registry Framework
tools/registry.py— Core framework:Tooldataclass (name, description, pydantic args_schema, handler, timeout)ToolRegistryexplicit allowlist (no auto-discovery)model_json_schema()→ GeminiFunctionDeclaration🛠️ Initial Tools
tools/handlers.py— Three read-only tools:calculate_zakat(public_key, nisab_usd?)— Reusesfetch_usdc_balance()and zakat math fromstellar.pyget_stellar_info()— Wraps Stellar network configurationsearch_courses(query)— HTTP stub to dnb-backend (BACKEND_API_URL)stellar.py— Refactored to share pure computation withtools/handlers.py; the REST endpoint callscompute_zakat()which delegates to_calculate_zakat()🌐 API Integration
main.py—/chatendpoint:tools=tool_registry.declarations()_run_with_tools()function handles function-calling loop (max 4 rounds)run_tool_handler()function_responseon validation failure, timeout, or exceptionChatResponse.tool_callsoptional field for frontend transparency🧪 Tests
tests/test_tools.py— 14 offline tests with fake model client:$schema,$defs,title)all/declarationsaccessToolCallRecordmodel creation🔧 Configuration
.github/workflows/ci.yml— Linttools/, compiletools/, run tool testsREADME.md— DocumentedBACKEND_API_URLenv vartests/conftest.py— SetGEMINI_API_KEYplaceholder for offline test importsAcceptance Criteria
calculate_zakattool; answer includes disclaimerSummary by CodeRabbit