-
Notifications
You must be signed in to change notification settings - Fork 0
refactor(models): Refine MessageAgentThought SQLAlchemy typing #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devin_pr_code_review_bench_100_devin2_base_refactormodels_refine_messageagentthought_sqlalchemy_typing_pr425
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| tool_call_response.append( | ||
| ToolPromptMessage( | ||
| content=tool_responses.get(tool, agent_thought.observation), | ||
| content=str(tool_inputs.get(tool, agent_thought.observation)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 ToolPromptMessage content uses tool_inputs instead of tool_responses
The ToolPromptMessage content is incorrectly populated with tool inputs instead of tool responses/observations.
Click to expand
Bug Details
In the organize_agent_history method, when constructing ToolPromptMessage objects for the agent history, the code incorrectly uses tool_inputs instead of tool_responses for the content.
The code at line 478-485 correctly parses the observation payload into tool_responses:
observation_payload = agent_thought.observation
if observation_payload:
try:
tool_responses = json.loads(observation_payload)
except Exception:
tool_responses = dict.fromkeys(tool_names, observation_payload)
else:
tool_responses = dict.fromkeys(tool_names, observation_payload)However, at line 502, the ToolPromptMessage content uses tool_inputs instead of tool_responses:
ToolPromptMessage(
content=str(tool_inputs.get(tool, agent_thought.observation)), # BUG: should be tool_responses
name=tool,
tool_call_id=tool_call_id,
)Expected Behavior
The ToolPromptMessage content should contain the tool's response/observation (what the tool returned), not the tool's input (what was sent to the tool).
Actual Behavior
The content contains the tool's input arguments, which is semantically incorrect and will cause the LLM to receive wrong context about what the tool actually returned.
Impact
This bug corrupts the agent's conversation history, causing the LLM to see tool inputs where it should see tool outputs. This can lead to incorrect agent behavior and responses in subsequent turns.
Recommendation: Change tool_inputs.get(tool, agent_thought.observation) to tool_responses.get(tool, agent_thought.observation) on line 502.
Was this helpful? React with 👍 or 👎 to provide feedback.
| answer_unit_price=Decimal("0.001"), | ||
| answer_price_unit=Decimal(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Swapped values for answer_unit_price and answer_price_unit in create_agent_thought
The initial values for answer_unit_price and answer_price_unit are swapped when creating a new MessageAgentThought.
Click to expand
Bug Details
In the create_agent_thought method, the values for answer_unit_price and answer_price_unit are incorrectly swapped:
answer_unit_price=Decimal("0.001"), # Should be Decimal(0)
answer_price_unit=Decimal(0), # Should be Decimal("0.001")Expected Behavior
Based on the naming convention and usage patterns elsewhere in the codebase (e.g., api/tests/unit_tests/services/test_conversation_service.py:230-232):
answer_unit_price= the price per unit for answers (should be0initially, updated later with actual pricing)answer_price_unit= the unit of price measurement like 0.001 for per 1000 tokens (should beDecimal("0.001")as the default)
The model definition at api/models/model.py:1872-1873 confirms answer_price_unit has a default of Decimal("0.001").
Actual Behavior
The values are swapped, setting the unit price to 0.001 and the price unit to 0.
Impact
This will cause incorrect pricing calculations for agent thoughts. The answer_price_unit being 0 means any price calculation using this field will result in 0 or incorrect values, potentially affecting billing and usage tracking.
Recommendation: Swap the values:
answer_unit_price=Decimal(0),
answer_price_unit=Decimal("0.001"),Was this helpful? React with 👍 or 👎 to provide feedback.
Benchmark PR from qodo-benchmark#425