LCORE-1166: Added tool calls and timestamps into turn history #1096
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR introduces structured, turn-level conversation modeling: new Message and ConversationTurn response schemas, a UserTurn DB model, utilities to build ConversationTurn lists from items/cache, updates persistence to record per-turn metadata, and removes the legacy conversations endpoint module. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
asimurka
force-pushed
the
v1/conversations_history_enrichment
branch
from
0581fdd to
0d571db
Compare
| @@ -84,7 +88,6 @@ | |||
| 500: InternalServerErrorResponse.openapi_response( | |||
| examples=["database", "configuration"] | |||
| ), | |||
| 503: ServiceUnavailableResponse.openapi_response(), | |||
There was a problem hiding this comment.
Does not access llama-stack.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/endpoints/query.py (1)
111-142:⚠️ Potential issue | 🔴 CriticalTurn-number allocation is race-prone under concurrent requests.
max(turn_number) + 1without a lock can produce duplicate turn numbers when overlapping requests hit the same conversation, leading to integrity errors and 500s. Serialize the allocation (lock the conversation row or use a sequence/retry) to make this deterministic.One possible fix (serialize with a row lock)
if not existing_conversation: conversation = UserConversation( id=normalized_id, user_id=user_id, last_used_model=model_id, last_used_provider=provider_id, topic_summary=topic_summary, message_count=1, ) session.add(conversation) + session.flush() # ensure row exists for locking/turn allocation logger.debug( "Associated conversation %s to user %s", normalized_id, user_id ) else: existing_conversation.last_used_model = model_id existing_conversation.last_used_provider = provider_id existing_conversation.last_message_at = datetime.now(UTC) existing_conversation.message_count += 1 logger.debug( "Updating existing conversation in DB - ID: %s, User: %s, Messages: %d", normalized_id, user_id, existing_conversation.message_count, ) + # Serialize turn-number allocation per conversation + session.query(UserConversation).filter_by(id=normalized_id).with_for_update().one() # Get the next turn number for this conversation max_turn_number = ( session.query(func.max(UserTurn.turn_number)) .filter_by(conversation_id=normalized_id) .scalar() )
🤖 Fix all issues with AI agents
In `@docs/openapi.json`:
- Around line 7123-7131: The Message schema's description is out of sync with
its enum: update the "description" string for the Message model to enumerate all
allowed types (system, user, assistant, developer) instead of saying only "user
or assistant"; locate the "Message" title/description block in the OpenAPI
schema (the object with "title": "Message" and "required": ["content","type"])
and modify the description to list or phrase the full enum values so it matches
the "type" enum.
- Around line 6017-6032: The OpenAPI schema for the turn timestamps is missing
the date-time format—update the "started_at" and "completed_at" properties in
the document (the JSON objects named "started_at" and "completed_at") to include
"format": "date-time" alongside "type": "string" so clients can validate these
as ISO 8601 date-time values; leave the existing titles, descriptions and
examples unchanged.
In `@src/app/endpoints/conversations_v1.py`:
- Around line 295-315: The code currently fetches db_turns and then calls
build_conversation_turns_from_items which can raise an IndexError for legacy
conversations when db_turns is empty or shorter than parsed items; add an
explicit guard after fetching db_turns to detect empty or insufficient metadata
(compare len(db_turns) to expected turns length from items or ensure db_turns is
non-empty), and if the check fails return a clear error (e.g., raise
HTTPException using a specific response like
InternalServerErrorResponse.database_error() or a new BadRequest/NotFound
response) including normalized_conv_id in the log, or alternatively provide a
safe fallback (e.g., treat missing metadata as empty turns) before calling
build_conversation_turns_from_items to avoid IndexError.
In `@src/models/responses.py`:
- Around line 838-855: The Message class docstring is out of date: update the
class-level docstring for class Message (and its Attributes section) to list all
allowed message types used by the type Field
(Literal["user","assistant","system","developer"]) so it mentions user,
assistant, system, and developer; ensure the description of the type attribute
matches the Field description for type in Message and adjust any examples or
wording to reflect all four allowed values.
In `@src/utils/endpoints.py`:
- Around line 822-829: The call to persist_user_conversation_details_func is
passing an unexpected keyword `model=`; update the argument to use the correct
parameter name `model_id=` (i.e., change the argument in the
persist_user_conversation_details_func invocation from model=model_id to
model_id=model_id) so it matches the function signature of
persist_user_conversation_details_func and avoids the TypeError.
In `@tests/unit/app/endpoints/test_conversations.py`:
- Around line 134-136: Update the inaccurate version note in the inline comment
above the datetime parsing: change the comment near the mock_turn.started_at and
mock_turn.completed_at assignments to state that datetime.fromisoformat supports
the "Z" suffix in Python 3.11+, i.e. replace "Python 3.12+" with "Python 3.11+"
so the comment accurately reflects when "Z" support was introduced.
🧹 Nitpick comments (2)
tests/unit/app/endpoints/test_conversations.py (2)
368-396: Remove unusedmock_session_dataparameter.The
mock_session_datafixture parameter on line 372 is unused in this test (as indicated by the pylint disable comment). Consider removing it from the test signature to keep the test clean.♻️ Proposed fix to remove unused parameter
`@pytest.mark.asyncio` async def test_build_conversation_turns_from_items_with_model_dump( self, mocker: MockerFixture, - mock_session_data: dict[str, Any], # pylint: disable=unused-argument expected_chat_history: list[dict[str, Any]], ) -> None:
1397-1417: Consider testing exception handling more explicitly.The
test_database_exceptiontest raises a genericExceptionand expects it to bubble up directly (pytest.raises(Exception, match="Database error")). This tests that unhandled exceptions propagate, but the endpoint may not have proper error handling for generic exceptions (unlikeSQLAlchemyError).If the endpoint should wrap all database exceptions in an HTTPException, this test would fail silently and potentially mask missing error handling.
| conversation = UserConversation( | ||
| id=normalized_id, | ||
| user_id=user_id, | ||
| last_used_model=model, |
There was a problem hiding this comment.
Just unification with outer scope variable names.
| topic_summary=topic_summary, | ||
| ) | ||
|
|
||
| completed_at = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ") |
There was a problem hiding this comment.
Moved before persisting
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/routers.py (1)
52-61:⚠️ Potential issue | 🟡 MinorUpdate the deprecation note to reference conversations_v1.
Line 60 still mentions
conversations_v3, which no longer matches the /v1 wiring.✅ Suggested fix
-# Note: query_v2, streaming_query_v2, and conversations_v3 are now exposed at /v1 above +# Note: query_v2, streaming_query_v2, and conversations_v1 are now exposed at /v1 above
🤖 Fix all issues with AI agents
In `@docs/openapi.json`:
- Around line 7096-7131: The Message schema's type property description is
incorrect: it says "user or assistant" while the enum includes "user",
"assistant", "system", and "developer"; update the description on the Message
schema's "type" property to list or otherwise reflect all four allowed enum
values (user, assistant, system, developer) to keep the documentation consistent
with the enum in the Message definition.
- Around line 5975-6040: The ConversationTurn schema currently lists properties
messages, tool_calls, and tool_results but doesn't mark them as required; decide
whether these arrays are always returned and then update the schema accordingly:
if the API always returns these arrays (possibly empty) add "messages",
"tool_calls", and "tool_results" to the ConversationTurn "required" array;
otherwise update each property's description (messages, tool_calls,
tool_results) to explicitly state they are optional/may be omitted and provide
example behavior (e.g., absent vs. empty array) so clients know the contract.
In `@src/app/endpoints/conversations_v2.py`:
- Around line 268-275: ConversationTurn instantiation fails when CacheEntry has
null started_at/completed_at; update the cache-loading logic (in the functions
that construct CacheEntry and in build_conversation_turn_from_cache_entry) to
supply defensive fallbacks for entry.started_at and entry.completed_at when they
are None (e.g., use entry.created_at or entry.updated_at, or DateTime.now() as
last resort) before creating the ConversationTurn; alternatively implement a
migration/backfill step to populate missing timestamps in persisted rows, but
for immediate safety add the fallback logic where CacheEntry is read in
sqlite_cache and postgres_cache and in build_conversation_turn_from_cache_entry
so Pydantic validation does not fail.
In `@src/app/endpoints/query.py`:
- Around line 136-152: The turn_number is computed from
max(UserTurn.turn_number) without serialization, causing race conditions; before
computing max and inserting UserTurn in the endpoint handling (where UserTurn is
created and session.add(turn)), perform a row-level lock on the
conversation/UserTurn rows for that conversation (e.g., issue a query with
with_for_update() filtering by conversation_id or lock the Conversation row) to
serialize concurrent requests, then recompute max(turn_number) and insert the
new UserTurn; ensure the with_for_update() call is applied on the same
session/transaction used to add/commit the UserTurn.
In `@src/utils/conversations.py`:
- Around line 305-343: The code currently assumes turns_metadata has an entry
for every turn and calls
_create_turn_from_db_metadata(turns_metadata[current_turn_index], ...) which can
raise IndexError; add a bounds check before each use of
turns_metadata[current_turn_index] (both inside the loop when finishing a turn
and when appending the final turn) and if the index is out of range either
backfill a minimal metadata object (e.g., with defaults for turn id/timestamp)
or skip/mark the turn as legacy so the function returns gracefully; ensure
current_turn_index is only incremented after confirming metadata exists and use
a helper like _get_turn_metadata_or_default(turns_metadata, current_turn_index)
to centralize the fallback logic referenced by _create_turn_from_db_metadata.
🧹 Nitpick comments (2)
src/app/endpoints/conversations_v2.py (1)
246-257: Align docstring sections with the repo’s “Parameters” convention.This function uses “Args:” rather than “Parameters:”, which deviates from the project docstring format.
As per coding guidelines: Follow Google Python docstring conventions including Parameters, Returns, Raises, and Attributes sections as needed.✏️ Suggested edit
- Args: + Parameters: entry: Cache entry representing one turn in the conversationdocs/openapi.json (1)
6017-6032: Adddate-timeformat for turn timestamps.You already state ISO 8601 in the description; adding
format: date-timeimproves generated client validation and tooling.🕒 Suggested schema tweak
"started_at": { "type": "string", + "format": "date-time", "title": "Started At", "description": "ISO 8601 timestamp when the turn started", "examples": [ "2024-01-01T00:01:00Z" ] }, "completed_at": { "type": "string", + "format": "date-time", "title": "Completed At", "description": "ISO 8601 timestamp when the turn completed", "examples": [ "2024-01-01T00:01:05Z" ] }
|
|
||
| # Turn number (1-indexed, first turn is 1) for ordering within a conversation | ||
| # Part of composite primary key with conversation_id | ||
| turn_number: Mapped[int] = mapped_column(primary_key=True) |
There was a problem hiding this comment.
Each conversation has its turns numbered by 1,2,3... easy to order and merge with conversation items then
asimurka
force-pushed
the
v1/conversations_history_enrichment
branch
2 times, most recently
from
7342054 to
bf91d2c
Compare
asimurka
force-pushed
the
v1/conversations_history_enrichment
branch
from
bf91d2c to
2e83485
Compare
1 participant
Description
This PR unifies and extends response models GET conversations/{conversation_id}.
Now the chat_history attribute contains a list of turns of the given conversation, and each turn contains the following attributes: started_at, completed_at, model, provider, tool_calls, tool_results, messages.
Turn metadata is stored in local persistent storage, while the conversation items themselves are obtained using the conversations API.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests