feat(llm): handle LLM tool call responses with execution loop #137

Merged

fuzzy merged 4 commits from fix/0095-handle-tool-calls into main 2026-07-05 17:40:44 +00:00

Conversation 2 Commits 4 Files changed 4 +158 -49

automation commented 2026-07-05 16:51:43 +00:00

Owner

Copy link

What

Handle LLM tool call responses: detect tool_calls in responses, execute tools via MCP servers, send results back to the LLM, and return the final text response.

Why

Previously, tool call responses with null content would return the string "None". Now tool calls are properly executed in a multi-turn loop.

Changes

• _extract_text handles None content gracefully
• complete() accepts execute_tool callback for tool call loop
• MCPServer.call_tool() for executing MCP tool calls
• Engine wires MCP servers to LLM tool execution

Testing

• 145 unit tests pass
• Pre-commit hooks pass

Closes #95

## What Handle LLM tool call responses: detect `tool_calls` in responses, execute tools via MCP servers, send results back to the LLM, and return the final text response. ## Why Previously, tool call responses with null `content` would return the string `"None"`. Now tool calls are properly executed in a multi-turn loop. ## Changes - `_extract_text` handles `None` content gracefully - `complete()` accepts `execute_tool` callback for tool call loop - `MCPServer.call_tool()` for executing MCP tool calls - Engine wires MCP servers to LLM tool execution ## Testing - [x] 145 unit tests pass - [x] Pre-commit hooks pass Closes #95

automation added 1 commit 2026-07-05 16:51:43 +00:00

feat(llm): handle LLM tool call responses with execution loop ... 74f2ce9416

- _extract_text handles None content gracefully
- complete() accepts execute_tool callback for tool call loop
- MCPServer.call_tool() for executing MCP tool calls
- Engine wires MCP servers to LLM tool execution
Closes #95

fuzzy force-pushed fix/0095-handle-tool-calls from 74f2ce9416 to 2e4c8cd328 2026-07-05 16:52:07 +00:00 Compare

fuzzy force-pushed fix/0095-handle-tool-calls from 2e4c8cd328 to d0b8e3324d 2026-07-05 16:55:45 +00:00 Compare

fuzzy force-pushed fix/0095-handle-tool-calls from d0b8e3324d to 649de4703b 2026-07-05 17:00:36 +00:00 Compare

fuzzy self-assigned this 2026-07-05 17:01:12 +00:00

fuzzy force-pushed fix/0095-handle-tool-calls from 649de4703b to ab0ad4ff7c 2026-07-05 17:05:10 +00:00 Compare

fuzzy force-pushed fix/0095-handle-tool-calls from ab0ad4ff7c to e3a1f1dae8 2026-07-05 17:13:19 +00:00 Compare

fuzzy force-pushed fix/0095-handle-tool-calls from e3a1f1dae8 to 55622a07d0 2026-07-05 17:19:58 +00:00 Compare

fuzzy force-pushed fix/0095-handle-tool-calls from 55622a07d0 to bae0bd821b 2026-07-05 17:24:18 +00:00 Compare

fuzzy force-pushed fix/0095-handle-tool-calls from bae0bd821b to 8e165c5fc5 2026-07-05 17:28:18 +00:00 Compare

fuzzy force-pushed fix/0095-handle-tool-calls from 8e165c5fc5 to 5babba5057 2026-07-05 17:30:46 +00:00 Compare

fuzzy referenced this pull request from a commit 2026-07-05 17:33:31 +00:00

refactor(engine): extract _init_mcp_servers to reduce run_job complexity

fuzzy added 1 commit 2026-07-05 17:33:31 +00:00

refactor(engine): extract _init_mcp_servers to reduce run_job complexity ... bafb3a353c

C901 cyclomatic complexity was 12. Extracting the MCP server
connection and tool collection logic brings it under the threshold.
Closes #137

the.auditor requested changes 2026-07-05 17:34:19 +00:00

Dismissed

the.auditor left a comment

Copy link

Automated Review — PR #137

Result: REQUEST_CHANGES

---

This is a substantial and important PR — the tool call execution loop enables proper multi-turn LLM conversations with MCP tool integration. The overall design is sound, but there is one blocking issue.

Good parts

• _send_and_parse() cleanly separates HTTP from response parsing
• _extract_text() handles None content gracefully (fixes the "None" string bug)
• MCPServer.call_tool() follows the existing _send_request pattern
• _init_mcp_servers() refactor is a nice cleanup
• Tool call loop has a safety limit (10 rounds) and proper message structure

Blocking Issue

1. src/kronai/engine.py:86-92 — MCPError during tool execution leaks server connections.

The _execute_tool closure calls server.call_tool() which can raise MCPError. This propagates through complete() and out, but run_job() only catches LLMError:

try:
    response = llm_client.complete(..., execute_tool=_execute_tool)
except LLMError:
    ...
    _cleanup_mcps(list(mcps.values()))
    return False

If the MCP server fails mid-tool-call, MCPError bypasses the cleanup and status/notification calls. The subprocess is leaked.

Options to fix:

• Option A: Have _execute_tool catch MCPError and raise LLMError instead (or return an error string)
• Option B: Add except (LLMError, MCPError, JobError) to the outer handler
• Option C: Have complete() accept a broader exception type or wrap tool execution errors

Non-blocking suggestions

2. src/kronai/engine.py:90 — str(server.call_tool(...)) — call_tool already returns str. Redundant str() call is harmless but unnecessary.

3. src/kronai/llm.py:96 — max_tool_rounds = 10 is hardcoded. Consider making it a constructor parameter (like max_retries and base_delay in PR #130).

## Automated Review — PR #137 **Result: REQUEST_CHANGES** --- This is a substantial and important PR — the tool call execution loop enables proper multi-turn LLM conversations with MCP tool integration. The overall design is sound, but there is one blocking issue. ### Good parts - `_send_and_parse()` cleanly separates HTTP from response parsing - `_extract_text()` handles None content gracefully (fixes the `"None"` string bug) - `MCPServer.call_tool()` follows the existing `_send_request` pattern - `_init_mcp_servers()` refactor is a nice cleanup - Tool call loop has a safety limit (10 rounds) and proper message structure ### Blocking Issue 1. **`src/kronai/engine.py:86-92`** — MCPError during tool execution leaks server connections. The `_execute_tool` closure calls `server.call_tool()` which can raise `MCPError`. This propagates through `complete()` and out, but `run_job()` only catches `LLMError`: ```python try: response = llm_client.complete(..., execute_tool=_execute_tool) except LLMError: ... _cleanup_mcps(list(mcps.values())) return False ``` If the MCP server fails mid-tool-call, `MCPError` bypasses the cleanup and status/notification calls. The subprocess is leaked. **Options to fix:** - **Option A:** Have `_execute_tool` catch `MCPError` and raise `LLMError` instead (or return an error string) - **Option B:** Add `except (LLMError, MCPError, JobError)` to the outer handler - **Option C:** Have `complete()` accept a broader exception type or wrap tool execution errors ### Non-blocking suggestions 2. **`src/kronai/engine.py:90`** — `str(server.call_tool(...))` — `call_tool` already returns `str`. Redundant `str()` call is harmless but unnecessary. 3. **`src/kronai/llm.py:96`** — `max_tool_rounds = 10` is hardcoded. Consider making it a constructor parameter (like `max_retries` and `base_delay` in PR #130).

the.auditor referenced this pull request 2026-07-05 17:34:59 +00:00

engine: redundant str() wrapping around call_tool return value #146

the.auditor referenced this pull request 2026-07-05 17:35:00 +00:00

llm: max_tool_rounds is hardcoded, should be configurable #147

fuzzy added 1 commit 2026-07-05 17:38:16 +00:00

fix: address PR review - remove redundant str(), make max_tool_rounds configurable ... d1ce3428ab

- Remove redundant str() wrapping around call_tool return (#146)
- Make max_tool_rounds a constructor parameter on LLMClient (#147)

fuzzy referenced this pull request from a commit 2026-07-05 17:39:13 +00:00

fix(engine): catch MCPError in _execute_tool to prevent connection leaks

fuzzy added 1 commit 2026-07-05 17:39:14 +00:00

fix(engine): catch MCPError in _execute_tool to prevent connection leaks ... bdb2476204

Tool execution failures now raise LLMError, ensuring the existing
error handler in run_job cleans up MCP connections and saves
status.
Addresses PR #137 review: blocking issue

the.auditor approved these changes 2026-07-05 17:40:10 +00:00

the.auditor left a comment

Copy link

Automated Review — PR #137 (re-review)

Result: APPROVED

---

Blocking issue resolved. _execute_tool now wraps MCPError in LLMError, so tool execution failures are caught by the existing cleanup/status/notification handler in run_job(). Server connections are no longer leaked.

Additionally, max_tool_rounds is now a constructor parameter matching the pattern from max_retries/base_delay. Redundant str() tracked in #146.

No remaining blocking issues.

## Automated Review — PR #137 (re-review) **Result: APPROVED** --- Blocking issue resolved. `_execute_tool` now wraps `MCPError` in `LLMError`, so tool execution failures are caught by the existing cleanup/status/notification handler in `run_job()`. Server connections are no longer leaked. Additionally, `max_tool_rounds` is now a constructor parameter matching the pattern from `max_retries`/`base_delay`. Redundant `str()` tracked in #146. No remaining blocking issues.

fuzzy scheduled this pull request to auto merge when all checks succeed 2026-07-05 17:40:31 +00:00

fuzzy merged commit bdb2476204 into main 2026-07-05 17:40:44 +00:00

fuzzy deleted branch fix/0095-handle-tool-calls 2026-07-05 17:40:44 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

the.auditor

Labels

Clear labels   

Compat/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

No labels

Compat/Breaking

Kind/Bug

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

fuzzy

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

thwap/kronai!137

No description provided.