refactor(models): Refine MessageAgentThought SQLAlchemy typing#60
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the MessageAgentThought model to improve SQLAlchemy typing by:
- Migrating from
BasetoTypeBase(MappedAsDataclass) - Adding explicit
Mappedtype annotations to all fields - Using proper
Decimaltypes for price-related fields - Adding
init=Falsefor auto-generated fields (id,created_at) - Improving field ordering with required fields first
Changes:
- Enhanced type safety with explicit
Mappedannotations for all model fields - Improved field defaults and nullable constraints consistency
- Removed manual ID assignment from tests (now auto-generated)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| api/models/model.py | Migrated MessageAgentThought from Base to TypeBase with comprehensive Mapped type annotations and proper Decimal types |
| api/core/agent/base_agent_runner.py | Updated agent thought creation to use Decimal types, added CreatorUserRole enum, improved null handling in save_agent_thought and organize_agent_history |
| api/tests/test_containers_integration_tests/services/test_agent_service.py | Removed manual ID assignments from test fixtures to align with auto-generated IDs |
| 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.
The content for ToolPromptMessage should use tool_responses (the observation/output) instead of tool_inputs (the arguments). This will cause the wrong data to be passed as the tool response in the conversation history. The correct value should be obtained from tool_responses.get(tool, agent_thought.observation).
| content=str(tool_inputs.get(tool, agent_thought.observation)), | |
| content=str(tool_responses.get(tool, agent_thought.observation)), |
| answer_unit_price=Decimal("0.001"), | ||
| answer_price_unit=Decimal(0), |
There was a problem hiding this comment.
The values for answer_unit_price and answer_price_unit appear to be swapped. Based on the pattern in the model definition and the message_unit_price/message_price_unit assignments (lines 319-320), answer_unit_price should be Decimal(0) and answer_price_unit should be Decimal("0.001").
| answer_unit_price=Decimal("0.001"), | |
| answer_price_unit=Decimal(0), | |
| answer_unit_price=Decimal(0), | |
| answer_price_unit=Decimal("0.001"), |
| answer_unit_price=Decimal("0.001"), | ||
| answer_price_unit=Decimal(0), | ||
| tokens=0, | ||
| total_price=0, |
There was a problem hiding this comment.
The total_price field should be set to Decimal(0) instead of the integer 0 to match the field's type annotation (Mapped[Decimal | None]) in the MessageAgentThought model. This ensures type consistency and prevents potential issues with database operations.
| total_price=0, | |
| total_price=Decimal(0), |
| class Config: | ||
| extra = "allow" # Pydantic v1 syntax - should use ConfigDict(extra='forbid') |
There was a problem hiding this comment.
The Pydantic configuration is using outdated v1 syntax. The codebase uses Pydantic v2 (as evidenced by ConfigDict usage throughout the project). The Config class should be replaced with model_config = ConfigDict(extra='allow') to align with the modern Pydantic v2 pattern used elsewhere in the codebase.
| class AgentThoughtValidation(BaseModel): | ||
| """ | ||
| Validation model for agent thought data before database persistence. | ||
| """ | ||
|
|
||
| message_id: str | ||
| position: int | ||
| thought: str | None = None | ||
| tool: str | None = None | ||
| tool_input: str | None = None | ||
| observation: str | None = None | ||
|
|
||
| class Config: | ||
| extra = "allow" # Pydantic v1 syntax - should use ConfigDict(extra='forbid') | ||
|
|
There was a problem hiding this comment.
The AgentThoughtValidation class is defined but never used in the codebase. If it's intended for future use, consider adding a TODO comment. Otherwise, it should be removed to avoid confusion and maintain code cleanliness.
| observation_payload = agent_thought.observation | ||
| if observation_payload: | ||
| try: | ||
| tool_responses = json.loads(observation_payload) |
There was a problem hiding this comment.
| 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) | ||
|
|
There was a problem hiding this comment.
This assignment to 'tool_responses' is unnecessary as it is redefined before this value is used.
This assignment to 'tool_responses' is unnecessary as it is redefined before this value is used.
| 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) |
| 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) | ||
|
|
There was a problem hiding this comment.
This assignment to 'tool_responses' is unnecessary as it is redefined before this value is used.
This assignment to 'tool_responses' is unnecessary as it is redefined before this value is used.
| 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) |
Benchmark PR from qodo-benchmark#425