Skip to content

Fix toolInUse initialization to correctly detect tool usage#425

Open
Nepomuk5665 wants to merge 1 commit into
2FastLabs:mainfrom
Nepomuk5665:fix/toolInUse-initialization
Open

Fix toolInUse initialization to correctly detect tool usage#425
Nepomuk5665 wants to merge 1 commit into
2FastLabs:mainfrom
Nepomuk5665:fix/toolInUse-initialization

Conversation

@Nepomuk5665

Copy link
Copy Markdown

Summary

This PR fixes a bug in bedrock_llm_agent.py where the toolInUse variable was incorrectly initialized to True before the loop that checks for tool usage, making it always True regardless of whether a tool was actually used.

Bug Description

In both handle_single_response and handle_streaming_response methods:

# Before fix (incorrect)
toolInUse=True  # Bug: always True
for item in response_content:
    if isinstance(item, dict) and "toolUse" in item:
        content.append(item)
        toolInUse = True  # Redundant, already True

This caused reasoningContent (thinking content) to always be inserted at index 0 of the content array, even when no tool was being used. According to the code comment, reasoning content should only be inserted at index 0 when a tool is used, and appended otherwise.

Fix

  • Changed toolInUse = True to toolInUse = False to correctly initialize the variable
  • Added a check for thinking_content/accumulated_thinking before appending to avoid appending None values when there is no thinking content
  • Updated comment to correctly describe the loop's purpose (saving "toolUse items" not "text items")

Impact

When extended thinking is enabled and no tool is used, the reasoningContent will now be correctly appended to the content array instead of being inserted at the beginning.

The toolInUse variable was incorrectly initialized to True before the
loop that checks for tool usage, making it always True regardless of
whether a tool was actually used. This caused reasoningContent to always
be inserted at index 0 instead of being appended when no tool is used.

Also added a check for thinking_content/accumulated_thinking before
appending to avoid appending None values when there is no thinking
content.
@cornelcroi

Copy link
Copy Markdown
Collaborator

Hey @Nepomuk5665, the fix looks correct — `toolInUse=True` on initialization made the `else` branch unreachable, and the `None`-guard for `reasoningContent` is a good catch. Two things are needed before this can merge: a linked issue (`pr-issue-link-checker` will fail without `Fixes #N` or `Closes #N` in the body) and CI must run and pass.

A short unit test covering the `toolInUse` logic would also be expected for a behavioral fix like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants