feat(kosong): add catch_exception param to SimpleToolset.handle() #755
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds an opt-out mechanism in SimpleToolset.handle() to avoid wrapping tool execution exceptions into ToolRuntimeError, enabling callers to observe underlying exceptions directly.
Changes:
- Extends
SimpleToolset.handle()with acatch_exceptionparameter (defaultTrue). - Conditionally re-wraps tool exceptions as
ToolRuntimeErrorwhencatch_exception=True, otherwise re-raises. - Threads
catch_exceptioninto the spawned asyncio task.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return [tool.base for tool in self._tool_dict.values()] | ||
|
|
||
| def handle(self, tool_call: ToolCall) -> HandleResult: | ||
| def handle(self, tool_call: ToolCall, catch_exception: bool = True) -> HandleResult: |
There was a problem hiding this comment.
New behavior via catch_exception isn’t covered by existing tests. There are already unit tests for SimpleToolset.handle behavior (e.g., packages/kosong/tests/test_tool_call.py), so please add a test that exercises catch_exception=False and asserts the expected outcome (either exception propagation or the chosen non-raising alternative) to prevent regressions.
| except Exception as e: | ||
| return ToolResult(tool_call_id=tool_call.id, return_value=ToolRuntimeError(str(e))) | ||
| if catch_exception: | ||
| return ToolResult(tool_call_id=tool_call.id, return_value=ToolRuntimeError(str(e))) | ||
| else: | ||
| raise |
There was a problem hiding this comment.
catch_exception=False causes _call to re-raise arbitrary exceptions (line 114), which violates the Toolset.handle contract that it “MUST NOT raise any exception except for asyncio.CancelledError” (packages/kosong/src/kosong/tooling/init.py:351-353). This will also break kosong.step because future_done_callback only catches asyncio.CancelledError, so other exceptions will surface as unhandled task/callback exceptions (packages/kosong/src/kosong/init.py:136-143). Consider keeping handle non-raising and instead add a hook/strategy to customize how exceptions are converted into a ToolReturnValue (e.g., allow passing an error-mapper function) so callers can preserve the underlying exception info without throwing from the task.
| async def _call(catch_exception: bool = catch_exception) -> ToolResult: | ||
| try: | ||
| ret = await tool.call(arguments) | ||
| return ToolResult(tool_call_id=tool_call.id, return_value=ret) | ||
| except Exception as e: | ||
| return ToolResult(tool_call_id=tool_call.id, return_value=ToolRuntimeError(str(e))) | ||
| if catch_exception: | ||
| return ToolResult(tool_call_id=tool_call.id, return_value=ToolRuntimeError(str(e))) | ||
| else: | ||
| raise | ||
|
|
||
| return asyncio.create_task(_call()) | ||
| return asyncio.create_task(_call(catch_exception)) |
There was a problem hiding this comment.
The inner _call defines catch_exception as a default argument (line 106) and then the task invocation also passes catch_exception (line 116). This is redundant and makes the control flow harder to follow. Prefer a single source of truth (either close over the outer variable and call _call() with no args, or remove the default and require _call(catch_exception) everywhere).
Description
In
kosongSimpleToolset.handle, all exceptions are caught and re-wrapped withToolRuntimeError, this erases the real exception and stopped me from handling the exceptions.I added an
catch_exceptionparam (default toTrue).Checklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.