fix(crew): keep the call id when a parallel tool fails - #118
CaptainAni187 wants to merge 1 commit into
Conversation
_execute_single already catches every Exception and returns a ToolResult carrying
the call's id, so the fallback in _execute_parallel only ever sees a BaseException.
It built that result with an empty tool_call_id and name, which reaches the model as
{"role": "tool", "tool_call_id": ""}: a tool message has to name the call it
answers, so the request is malformed rather than just uninformative. gather preserves
order, so pair each result with its own call.
CancelledError is the BaseException that actually turns up here. Re-raise it instead
of turning it into a result, so a cancelled turn stops rather than reporting a
fabricated answer for the call.
|
crim doesn't review pull requests automatically here. Comment |
|
crim review |
|
crim is reviewing this pull request. Findings will be posted shortly. |
There was a problem hiding this comment.
LGTM
One-sentence assessment: A clean, well-tested bug fix that correctly preserves the tool call id/name on the parallel error path and re-raises cancellation instead of masking it.
What this PR does: In _execute_parallel, when asyncio.gather(return_exceptions=True) surfaces a BaseException (which _execute_single does not already convert to a ToolResult), the fallback previously built a ToolResult with tool_call_id="" and name="", producing a malformed tool message the LLM cannot match to its call. The fix zips each result back to its originating call to carry the real id/name, falls back to the exception type name when the message is empty, and re-raises asyncio.CancelledError so a cancelled turn does not proceed with a fabricated result. A new regression test covers all of these paths.
Findings: No issues found.
Good to mergeA fix that gives orphaned error results their id back and stops pretending a cancelled turn finished cleanly, with tests for every branch it touches. The empty-string tool_call_id can finally rest in peace. |
|
@CaptainAni187 still thank you for raising this! 🙌🏼 |
When a tool fails in parallel mode, the error
ToolResultis built with no id and no name:The call it belongs to is right there in
tool_calls, andgatherpreserves input order, so the pairing is available and just not used.That matters because these go straight back to the model as tool messages. A tool message has to name the call it answers, so an empty
tool_call_idmakes the request malformed rather than merely uninformative:Note the content is empty too.
str()on an exception constructed without a message is"", so the model is handed a tool message with nothing in it at all.How you reach that branch
Worth being precise, because it is narrower than it looks.
_execute_singlealready catches everyExceptionand returns a properToolResultwithtool_call_id=call.id, so ordinary tool failures never get here. Only aBaseExceptiondoes, and in practice that meansasyncio.CancelledError.Which is the second half of this. Converting a
CancelledErrorinto aToolResultmeans a cancelled turn does not stop: it carries on and reports a result for a call that never completed. For a voice agent, where an interruption cancelling in-flight work is routine rather than exceptional, that is the wrong end state.The change
zip(tool_calls, results)so an error result carries the id and name of its own call, and fall back to the exception's type name whenstr()on it is empty.CancelledErroris re-raised rather than converted. Cancellation should propagate.If you would rather not change the cancellation behaviour, the
zipalone still fixes the malformed message and I am happy to drop the re-raise.Tests
tests/custom/test_tool_registry_parallel_errors.py, six cases. Four fail onmain: the id and name survive, the content is never blank, results stay aligned with their calls across a mixed success/failure batch, and cancellation propagates. The other two pin what must not change, an ordinaryExceptionstill becoming a tool result with the right id, and the sequential path behaving as before.src/smallestai/atoms/crew/**is.fernignored, so a regen keeps this.