refactor(models): Refine MessageAgentThought SQLAlchemy typing#37
Conversation
WalkthroughRefactors agent thought processing with new Pydantic validation, switches to Decimal-based numeric handling, restructures the MessageAgentThought model with creator/chain tracking fields, and improves tool data parsing robustness in the organize_agent_history flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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:ToolPromptMessagecontent usestool_inputsinstead oftool_responses.The
ToolPromptMessagerepresents the tool's response/output, but line 502 retrieves content fromtool_inputsinstead oftool_responses. This appears semantically incorrect - the tool call response should contain the observation/output data, not the input data.🐛 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-326: The answer_unit_price and answer_price_unit are swapped
compared to the message_* pattern and model defaults; in base_agent_runner where
the row is constructed (fields message_unit_price, message_price_unit,
answer_unit_price, answer_price_unit), change the answer_* values to mirror the
message_* order so answer_unit_price=Decimal(0) and
answer_price_unit=Decimal("0.001") (matching the model.py defaults for
*_price_unit and the existing message_* assignments).
In `@api/models/model.py`:
- Around line 1856-1857: The mapped columns tool_labels_str and tool_meta_str
currently use default=sa.text("'{}'") which is incorrect; change them to use a
Python-side default default="{}" and, if a DB-level default is desired, add
server_default=sa.text("'{}'") instead of using sa.text() in the default
parameter so object instantiation gets the string "{}" and the database still
has the same fallback.
🧹 Nitpick comments (3)
api/models/model.py (1)
1864-1876: InconsistentNumericprecision specification for price fields.
message_unit_priceandanswer_unit_price(lines 1864, 1871) usesa.Numericwithout precision, whilemessage_price_unitandanswer_price_unit(lines 1865-1866, 1872-1873) specifysa.Numeric(10, 7). This inconsistency could lead to different storage behavior across databases and potential precision loss.♻️ Proposed fix to standardize precision
- message_unit_price: Mapped[Decimal | None] = mapped_column(sa.Numeric, nullable=True, default=None) + message_unit_price: Mapped[Decimal | None] = mapped_column(sa.Numeric(10, 7), nullable=True, default=None) message_price_unit: Mapped[Decimal] = mapped_column( sa.Numeric(10, 7), nullable=False, default=Decimal("0.001"), server_default=sa.text("0.001") ) message_files: Mapped[str | None] = mapped_column(LongText, nullable=True, default=None) answer: Mapped[str | None] = mapped_column(LongText, nullable=True, default=None) answer_token: Mapped[int | None] = mapped_column(sa.Integer, nullable=True, default=None) - answer_unit_price: Mapped[Decimal | None] = mapped_column(sa.Numeric, nullable=True, default=None) + answer_unit_price: Mapped[Decimal | None] = mapped_column(sa.Numeric(10, 7), nullable=True, default=None)api/core/agent/base_agent_runner.py (2)
52-66: Unused Pydantic model and inconsistent comment.
AgentThoughtValidationis defined but never instantiated or used anywhere in this file. Additionally, line 65's comment suggests usingextra='forbid'but the actual value is"allow", which is contradictory.Either:
- Remove this class if it's not needed
- Use it for validation before creating
MessageAgentThoughtinstances- Fix the comment to match the intended behavior
328-328: UseDecimalfortotal_pricefor type consistency.Other price-related fields use
Decimal, buttotal_price=0uses an integer literal. For consistency with the model'sMapped[Decimal | None]type, this should beDecimal(0).♻️ Proposed fix
- total_price=0, + total_price=Decimal(0),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/core/agent/base_agent_runner.pyapi/models/model.pyapi/tests/test_containers_integration_tests/services/test_agent_service.py
💤 Files with no reviewable changes (1)
- api/tests/test_containers_integration_tests/services/test_agent_service.py
🧰 Additional context used
🧬 Code graph analysis (2)
api/core/agent/base_agent_runner.py (1)
api/models/enums.py (1)
CreatorUserRole(6-8)
api/models/model.py (2)
api/models/base.py (1)
TypeBase(17-22)api/models/types.py (2)
StringUUID(15-38)LongText(41-61)
🔇 Additional comments (3)
api/models/model.py (1)
1838-1881: LGTM on the overall model refactoring.The transition to
TypeBase, addition of explicitMappedtype annotations, and the new creator/chain tracking fields (created_by_role,created_by,message_chain_id) align well with the SQLAlchemy 2.0 dataclass pattern shown in theTypeBasedefinition.api/core/agent/base_agent_runner.py (2)
364-366: LGTM on thought concatenation refactor.The updated logic properly handles the case where
agent_thought.thoughtmay beNoneby defaulting to an empty string before concatenation.
464-518: LGTM on the refactored tool parsing logic.The improved parsing with explicit
try/exceptblocks for JSON parsing and proper fallback handling (tool_inputsandtool_responsesdictionaries) makes the code more robust against malformed data.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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), |
There was a problem hiding this comment.
answer_unit_price and answer_price_unit values appear swapped.
The initialization pattern for message fields (lines 319-320) is:
message_unit_price=Decimal(0)message_price_unit=Decimal("0.001")
But for answer fields (lines 325-326):
answer_unit_price=Decimal("0.001")answer_price_unit=Decimal(0)
This is inconsistent. The model in model.py defines both *_price_unit fields with default=Decimal("0.001"), suggesting the values should be swapped.
🐛 Proposed fix
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"),📝 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), | |
| 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"), |
🤖 Prompt for AI Agents
In `@api/core/agent/base_agent_runner.py` around lines 319 - 326, The
answer_unit_price and answer_price_unit are swapped compared to the message_*
pattern and model defaults; in base_agent_runner where the row is constructed
(fields message_unit_price, message_price_unit, answer_unit_price,
answer_price_unit), change the answer_* values to mirror the message_* order so
answer_unit_price=Decimal(0) and answer_price_unit=Decimal("0.001") (matching
the model.py defaults for *_price_unit and the existing message_* assignments).
| 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("'{}'")) |
There was a problem hiding this comment.
Incorrect use of sa.text() with default parameter.
The default parameter in SQLAlchemy's mapped_column expects a Python value or callable, not an sa.text() expression. Using sa.text("'{}'") as default will not produce the intended "{}" string value during Python-side object instantiation - it will store the sa.text() object itself or cause type errors.
Use server_default=sa.text("'{}'") for database-level defaults and default="{}" for Python-side defaults.
🐛 Proposed fix
- 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="{}", server_default=sa.text("'{}'"))
+ tool_meta_str: Mapped[str] = mapped_column(LongText, nullable=False, default="{}", server_default=sa.text("'{}'"))📝 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.
| 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="{}", server_default=sa.text("'{}'")) | |
| tool_meta_str: Mapped[str] = mapped_column(LongText, nullable=False, default="{}", server_default=sa.text("'{}'")) |
🤖 Prompt for AI Agents
In `@api/models/model.py` around lines 1856 - 1857, The mapped columns
tool_labels_str and tool_meta_str currently use default=sa.text("'{}'") which is
incorrect; change them to use a Python-side default default="{}" and, if a
DB-level default is desired, add server_default=sa.text("'{}'") instead of using
sa.text() in the default parameter so object instantiation gets the string "{}"
and the database still has the same fallback.
1 participant
Benchmark PR from agentic-review-benchmarks#11
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.