refactor(models): Refine MessageAgentThought SQLAlchemy typing#11
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the MessageAgentThought SQLAlchemy model to use stricter typing with TypeBase (which extends MappedAsDataclass) instead of the plain Base class. The changes add comprehensive type annotations using Mapped[] for all fields, configure proper defaults, and adjust related code to work with the new model structure.
Changes:
- Converted
MessageAgentThoughtfromBasetoTypeBasewith fullMapped[]type annotations for all fields - Updated field definitions with explicit
defaultvalues and properly configured auto-generated ID field withinit=False - Modified
create_agent_thoughtand related methods inBaseAgentRunnerto use correct types (Decimal for prices, proper enums) - Removed explicit
idparameter from test fixtures since IDs are now auto-generated - Refactored
organize_agent_historymethod to improve null handling and variable naming
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| api/models/model.py | Converted MessageAgentThought to TypeBase with comprehensive Mapped[] type annotations, proper defaults, and field ordering for dataclass compatibility |
| api/core/agent/base_agent_runner.py | Updated agent thought creation/manipulation to use Decimal types, CreatorUserRole enum, added AgentThoughtValidation model, and refactored organize_agent_history for better null safety |
| api/tests/test_containers_integration_tests/services/test_agent_service.py | Removed explicit id assignments from MessageAgentThought instantiations in tests to work with auto-generated IDs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| total_price=0, | ||
| position=self.agent_thought_count + 1, | ||
| currency="USD", | ||
| latency=0, |
There was a problem hiding this comment.
Type inconsistency: The field latency is defined as Mapped[float | None] in the model (line 1878 of model.py), but here it's being assigned an integer value 0. While Python will implicitly convert this, for type consistency and clarity, consider using 0.0 instead.
| latency=0, | |
| latency=0.0, |
| 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.
Critical bug: The ToolPromptMessage content should use tool_responses.get(tool, ...) instead of tool_inputs.get(tool, ...). This will incorrectly send tool inputs as tool responses, causing the agent to receive the wrong information about tool execution results. The tool_responses dictionary was properly parsed from observation_payload on lines 478-485 but is not being used here.
| 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), | ||
| tokens=0, | ||
| total_price=0, |
There was a problem hiding this comment.
Type mismatch: The field total_price is defined as Mapped[Decimal | None] in the model (line 1876 of model.py), but here it's being assigned an integer value 0. This should be Decimal(0) to match the expected type, maintaining consistency with other Decimal fields like message_unit_price and answer_unit_price.
| total_price=0, | |
| total_price=Decimal(0), |
| message_unit_price=Decimal(0), | ||
| message_price_unit=Decimal("0.001"), |
There was a problem hiding this comment.
Potential value swap: The values for message_unit_price and message_price_unit appear to be swapped. Typically, unit_price represents the cost per unit (e.g., per token) which should be a small value like 0.001, while the price_unit denomination would be 0 initially. Based on the model defaults (lines 1864-1867 in model.py), message_unit_price should be 0 or None (nullable), and message_price_unit should be Decimal("0.001") (non-nullable with default). Consider swapping these values to match the expected pattern.
| answer_unit_price=Decimal("0.001"), | ||
| answer_price_unit=Decimal(0), |
There was a problem hiding this comment.
Potential value swap: The values for answer_unit_price and answer_price_unit appear to be swapped. Similar to the message fields above, answer_unit_price should typically be a small value like 0.001 or None (as it's nullable), while answer_price_unit should be 0 initially but defaults to Decimal("0.001") per the model definition. Based on the model defaults (lines 1871-1874 in model.py), consider swapping these values: answer_unit_price=Decimal(0) and answer_price_unit=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"), |
| class Config: | ||
| extra = "allow" # Pydantic v1 syntax - should use ConfigDict(extra='forbid') |
There was a problem hiding this comment.
Pydantic version inconsistency: The code uses Pydantic v1 syntax with class Config and extra = "allow", but the codebase has migrated to Pydantic v2 which uses model_config = ConfigDict(extra="allow"). For consistency with the rest of the codebase (see examples in api/core/mcp/types.py, api/core/app/app_config/features/more_like_this/manager.py, etc.), this should be updated to use the v2 syntax. Additionally, the comment mentions using extra='forbid', but the actual setting is extra='allow' - if validation is intended, consider whether 'forbid' would be more appropriate.
| 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 anywhere in the codebase. If this validation model is not intended to be used yet, consider removing it to avoid dead code. If it's planned for future use, consider adding a TODO comment explaining the intended usage.
| 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) |
| 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 agentic-review-benchmarks#11