refactor(models): Refine MessageAgentThought SQLAlchemy typing#11
Conversation
📝 WalkthroughWalkthroughThis pull request introduces stricter data validation and type safety for agent thought persistence. Changes include a new validation model for agent thoughts, restructured database schema for the MessageAgentThought entity with explicit field typing and defaults, improved monetary field handling using Decimal, and refactored tool history parsing logic. Test fixtures are updated to rely on auto-generated IDs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/core/agent/base_agent_runner.py (1)
500-506:⚠️ Potential issue | 🔴 CriticalBug:
tool_call_responseshould usetool_responses, nottool_inputs.The
ToolPromptMessagerepresents the tool's response/output, but line 502 retrieves content fromtool_inputsinstead oftool_responses. This causes the tool response to incorrectly contain the input data rather than the actual observation/output.Proposed fix
tool_call_response.append( ToolPromptMessage( - content=str(tool_inputs.get(tool, agent_thought.observation)), + content=str(tool_responses.get(tool, agent_thought.observation)), name=tool, tool_call_id=tool_call_id, ) )
🤖 Fix all issues with AI agents
In `@api/core/agent/base_agent_runner.py`:
- Around line 319-328: The answer_* and message_* price fields are likely
swapped and total_price has wrong type: in the initialization inside
BaseAgentRunner (or the class/constructor where message_unit_price,
message_price_unit, answer_unit_price, answer_price_unit are set) swap the
values so message_unit_price and answer_unit_price are Decimal(0) and
message_price_unit and answer_price_unit are Decimal("0.001"); also change
total_price from int 0 to a Decimal(0) (or None) to match the declared type
Mapped[Decimal | None]. Ensure you update the initialization of
answer_unit_price, answer_price_unit, message_unit_price, message_price_unit,
and total_price accordingly.
🧹 Nitpick comments (2)
api/models/model.py (1)
1856-1857: Consider using plain Python strings for defaults instead ofsa.text().Using
sa.text("'{}'")as thedefaultparameter is unconventional. While SQLAlchemy 2.0 accepts SQL expressions for client-side defaults, a plain Python string"{}"would be simpler and more readable. Thesa.text()approach is typically reserved forserver_default.Suggested simplification
- tool_labels_str: Mapped[str] = mapped_column(LongText, nullable=False, default=sa.text("'{}'")) - tool_meta_str: Mapped[str] = mapped_column(LongText, nullable=False, default=sa.text("'{}'")) + tool_labels_str: Mapped[str] = mapped_column(LongText, nullable=False, default="{}") + tool_meta_str: Mapped[str] = mapped_column(LongText, nullable=False, default="{}")api/core/agent/base_agent_runner.py (1)
52-66: UpdateAgentThoughtValidationto Pydantic v2 syntax.The model uses deprecated Pydantic v1
class Configsyntax. With pydantic 2.11.4, usemodel_config = ConfigDict(extra="allow")instead:from pydantic import BaseModel, ConfigDict 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 model_config = ConfigDict(extra="allow")This model is not referenced anywhere in the codebase—confirm if this is intentional or if integration is missing.
| message_unit_price=Decimal(0), | ||
| message_price_unit=Decimal("0.001"), | ||
| message_files=json.dumps(messages_ids) if messages_ids else "", | ||
| answer="", | ||
| observation="", | ||
| answer_token=0, | ||
| answer_unit_price=0, | ||
| answer_price_unit=0, | ||
| answer_unit_price=Decimal("0.001"), | ||
| answer_price_unit=Decimal(0), | ||
| tokens=0, | ||
| total_price=0, |
There was a problem hiding this comment.
Verify answer_unit_price and answer_price_unit values are not swapped.
The initialization values appear inconsistent between message and answer fields:
| Field | message_* | answer_* |
|---|---|---|
| unit_price | Decimal(0) |
Decimal("0.001") |
| price_unit | Decimal("0.001") |
Decimal(0) |
Typically, *_price_unit represents the unit size (e.g., 0.001 for per-1000 tokens) and should be non-zero, while *_unit_price is the actual price which starts at 0. The answer fields appear to have these values swapped.
Also, total_price=0 should be Decimal(0) or None for type consistency with Mapped[Decimal | None].
Suggested fix
message_unit_price=Decimal(0),
message_price_unit=Decimal("0.001"),
message_files=json.dumps(messages_ids) if messages_ids else "",
answer="",
observation="",
answer_token=0,
- answer_unit_price=Decimal("0.001"),
- answer_price_unit=Decimal(0),
+ answer_unit_price=Decimal(0),
+ answer_price_unit=Decimal("0.001"),
tokens=0,
- total_price=0,
+ total_price=Decimal(0),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message_unit_price=Decimal(0), | |
| message_price_unit=Decimal("0.001"), | |
| message_files=json.dumps(messages_ids) if messages_ids else "", | |
| answer="", | |
| observation="", | |
| answer_token=0, | |
| answer_unit_price=0, | |
| answer_price_unit=0, | |
| answer_unit_price=Decimal("0.001"), | |
| answer_price_unit=Decimal(0), | |
| tokens=0, | |
| total_price=0, | |
| message_unit_price=Decimal(0), | |
| message_price_unit=Decimal("0.001"), | |
| message_files=json.dumps(messages_ids) if messages_ids else "", | |
| answer="", | |
| observation="", | |
| answer_token=0, | |
| answer_unit_price=Decimal(0), | |
| answer_price_unit=Decimal("0.001"), | |
| tokens=0, | |
| total_price=Decimal(0), |
🤖 Prompt for AI Agents
In `@api/core/agent/base_agent_runner.py` around lines 319 - 328, The answer_* and
message_* price fields are likely swapped and total_price has wrong type: in the
initialization inside BaseAgentRunner (or the class/constructor where
message_unit_price, message_price_unit, answer_unit_price, answer_price_unit are
set) swap the values so message_unit_price and answer_unit_price are Decimal(0)
and message_price_unit and answer_price_unit are Decimal("0.001"); also change
total_price from int 0 to a Decimal(0) (or None) to match the declared type
Mapped[Decimal | None]. Ensure you update the initialization of
answer_unit_price, answer_price_unit, message_unit_price, message_price_unit,
and total_price accordingly.
2 participants
Benchmark PR from agentic-review-benchmarks#11
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor