-
Notifications
You must be signed in to change notification settings - Fork 45
[RHDHPAI-1064]Topic summary rename #642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
Warning Rate limit exceeded@yangcao77 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 45 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
WalkthroughAdds a GET /v1/tools OpenAPI path and ToolsResponse schema, extends Action enum with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as Conversations V2 API
participant Auth as Auth
participant Cache as ConversationCache
Client->>API: PUT /v2/conversations/{conversation_id}\n{ "topic_summary": "..." }
API->>Auth: check permission (update_conversation)
Auth-->>API: authorized / unauthorized
alt unauthorized
API-->>Client: 401/403
else authorized
API->>API: validate conversation_id format
alt invalid id
API-->>Client: 400
else valid id
API->>Cache: get conversation
alt not found
API-->>Client: 404
else found
API->>Cache: set_topic_summary(...)
Cache-->>API: ack
API-->>Client: 200 ConversationUpdateResponse
end
end
end
sequenceDiagram
autonumber
participant Client
participant API as Tools API (v1)
participant MCP as MCP Providers
Client->>API: GET /v1/tools
API->>MCP: aggregate tools from MCP servers
MCP-->>API: tools list
API-->>Client: 200 ToolsResponse { tools: [...] }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
Signed-off-by: Stephanie <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/openapi.json
(5 hunks)src/app/endpoints/conversations_v2.py
(3 hunks)src/models/config.py
(1 hunks)src/models/requests.py
(1 hunks)src/models/responses.py
(1 hunks)tests/integration/test_openapi_json.py
(1 hunks)tests/unit/app/endpoints/test_conversations_v2.py
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py
: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/integration/test_openapi_json.py
src/models/responses.py
src/models/config.py
src/models/requests.py
src/app/endpoints/conversations_v2.py
tests/unit/app/endpoints/test_conversations_v2.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py
: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/integration/test_openapi_json.py
tests/unit/app/endpoints/test_conversations_v2.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py
: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/integration/test_openapi_json.py
tests/unit/app/endpoints/test_conversations_v2.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/models/responses.py
src/models/config.py
src/models/requests.py
src/app/endpoints/conversations_v2.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}
: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/responses.py
src/models/config.py
src/models/requests.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py
: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/responses.py
src/models/config.py
src/models/requests.py
src/{models/config.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/config.py,configuration.py}
: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields
Files:
src/models/config.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Files:
src/app/endpoints/conversations_v2.py
src/{app/**/*.py,client.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Use async def for I/O-bound operations and external API calls
Files:
src/app/endpoints/conversations_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Files:
src/app/endpoints/conversations_v2.py
🧬 Code graph analysis (2)
src/app/endpoints/conversations_v2.py (10)
src/models/requests.py (1)
ConversationUpdateRequest
(415-438)src/models/responses.py (2)
ConversationUpdateResponse
(870-902)UnauthorizedResponse
(511-529)src/authorization/middleware.py (1)
authorize
(111-122)src/models/config.py (1)
Action
(327-371)src/utils/endpoints.py (1)
check_configuration_loaded
(68-80)src/cache/postgres_cache.py (1)
set_topic_summary
(347-374)src/cache/sqlite_cache.py (1)
set_topic_summary
(343-368)src/cache/cache.py (1)
set_topic_summary
(106-120)src/cache/in_memory_cache.py (1)
set_topic_summary
(105-121)src/cache/noop_cache.py (1)
set_topic_summary
(103-119)
tests/unit/app/endpoints/test_conversations_v2.py (5)
src/app/endpoints/conversations_v2.py (4)
transform_chat_message
(318-329)update_conversation_endpoint_handler
(239-284)check_valid_conversation_id
(287-297)check_conversation_existence
(300-315)src/models/cache_entry.py (1)
CacheEntry
(6-21)src/models/requests.py (1)
ConversationUpdateRequest
(415-438)src/models/responses.py (1)
ConversationUpdateResponse
(870-902)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers
(9-27)
🪛 GitHub Actions: Black
src/app/endpoints/conversations_v2.py
[error] 1-1: Black formatting check failed. 2 files would be reformatted. The command used was 'uv tool run black --check .'
tests/unit/app/endpoints/test_conversations_v2.py
[error] 1-1: Black formatting check failed. 2 files would be reformatted. The command used was 'uv tool run black --check .'
🪛 GitHub Actions: Python linter
tests/unit/app/endpoints/test_conversations_v2.py
[warning] 95-95: Trailing whitespace (trailing-whitespace).
[warning] 98-98: Trailing whitespace (trailing-whitespace).
[warning] 112-112: Trailing whitespace (trailing-whitespace).
[warning] 120-120: Trailing whitespace (trailing-whitespace).
[warning] 123-123: Trailing whitespace (trailing-whitespace).
[warning] 136-136: Trailing whitespace (trailing-whitespace).
[warning] 138-138: Trailing whitespace (trailing-whitespace).
[warning] 145-145: Trailing whitespace (trailing-whitespace).
[warning] 154-154: Trailing whitespace (trailing-whitespace).
[warning] 156-156: Trailing whitespace (trailing-whitespace).
[warning] 163-163: Trailing whitespace (trailing-whitespace).
[warning] 175-175: Trailing whitespace (trailing-whitespace).
[warning] 177-177: Trailing whitespace (trailing-whitespace).
[warning] 184-184: Trailing whitespace (trailing-whitespace).
[warning] 195-195: Trailing whitespace (trailing-whitespace).
[warning] 197-197: Trailing whitespace (trailing-whitespace).
[warning] 204-204: Trailing whitespace (trailing-whitespace).
[warning] 217-217: Trailing whitespace (trailing-whitespace).
[warning] 219-219: Trailing whitespace (trailing-whitespace).
[warning] 225-225: Trailing whitespace (trailing-whitespace).
[warning] 75-75: Unused argument 'mocker' (unused-argument).
[warning] 132-132: Unused argument 'dummy_request' (unused-argument).
[warning] 149-149: Unused argument 'dummy_request' (unused-argument).
[warning] 168-168: Unused argument 'dummy_request' (unused-argument).
[warning] 189-189: Unused argument 'dummy_request' (unused-argument).
[warning] 209-209: Unused argument 'dummy_request' (unused-argument).
[warning] 7-7: Standard import order issue: standard import 'unittest.mock.Mock' should be placed before third party imports (pytest, fastapi.HTTPException) (wrong-import-order).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e_tests (ci)
- GitHub Check: build-pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/endpoints/conversations_v2.py (1)
94-115
: Removetopic_summary
from the 200 response example.The 200 response example on line 99 includes
topic_summary
, but theConversationUpdateResponse
schema (lines 869-901 in src/models/responses.py) only definesconversation_id
,success
, andmessage
fields. Remove line 99 to align the example with the actual response model.Apply this diff:
conversation_update_responses: dict[int | str, dict[str, Any]] = { "conversation_id": "123e4567-e89b-12d3-a456-426614174000", "success": True, "message": "Topic summary updated successfully", - "topic_summary": "Updated topic summary", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/endpoints/conversations_v2.py
(3 hunks)tests/unit/app/endpoints/test_conversations_v2.py
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/app/endpoints/conversations_v2.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Files:
src/app/endpoints/conversations_v2.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py
: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/app/endpoints/conversations_v2.py
tests/unit/app/endpoints/test_conversations_v2.py
src/{app/**/*.py,client.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Use async def for I/O-bound operations and external API calls
Files:
src/app/endpoints/conversations_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Files:
src/app/endpoints/conversations_v2.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py
: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/app/endpoints/test_conversations_v2.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py
: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/app/endpoints/test_conversations_v2.py
🧬 Code graph analysis (2)
src/app/endpoints/conversations_v2.py (10)
src/models/requests.py (1)
ConversationUpdateRequest
(415-438)src/models/responses.py (2)
ConversationUpdateResponse
(870-902)UnauthorizedResponse
(511-529)src/models/config.py (1)
Action
(327-371)src/authentication/__init__.py (1)
get_auth_dependency
(14-52)src/utils/endpoints.py (1)
check_configuration_loaded
(68-80)src/cache/postgres_cache.py (1)
set_topic_summary
(347-374)src/cache/sqlite_cache.py (1)
set_topic_summary
(343-368)src/cache/cache.py (1)
set_topic_summary
(106-120)src/cache/noop_cache.py (1)
set_topic_summary
(103-119)src/cache/in_memory_cache.py (1)
set_topic_summary
(105-121)
tests/unit/app/endpoints/test_conversations_v2.py (4)
src/app/endpoints/conversations_v2.py (4)
transform_chat_message
(316-327)update_conversation_endpoint_handler
(237-282)check_valid_conversation_id
(285-295)check_conversation_existence
(298-313)src/models/requests.py (1)
ConversationUpdateRequest
(415-438)src/models/responses.py (1)
ConversationUpdateResponse
(870-902)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers
(9-27)
🪛 GitHub Actions: Python linter
tests/unit/app/endpoints/test_conversations_v2.py
[error] 63-63: pylint: W0613 Unused argument 'mocker' (unused-argument).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (5)
src/app/endpoints/conversations_v2.py (2)
13-13
: LGTM!The new imports for
ConversationUpdateRequest
andConversationUpdateResponse
are properly ordered and follow the existing import structure.Also applies to: 17-17
235-282
: LGTM!The endpoint implementation follows the established pattern in this file (configuration validation, ID checks, cache existence verification, operation, logging) and correctly integrates the new request/response models. The handler properly calls
set_topic_summary
with the appropriate parameters and returns a structured success response.tests/unit/app/endpoints/test_conversations_v2.py (3)
5-18
: LGTM!Import order now follows PEP 8 conventions (standard library → third-party → local), resolving the ordering issues from the previous review.
20-22
: LGTM!Test constants are properly defined at module level. The
MOCK_AUTH
constant correctly follows the shared pattern from coding guidelines.
71-224
: LGTM!The test suite comprehensively covers the new endpoint and helper functions:
- Helper function tests verify valid/invalid ID handling and conversation existence checks
- Endpoint tests cover all error paths (configuration not loaded, invalid ID, cache not configured, conversation not found) and the success case
- The success test properly verifies the side effect (cache update call) with correct parameters
The test structure follows pytest best practices and aligns with coding guidelines.
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
…to topic_summary_rename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice one
Description
for issue https://issues.redhat.com/browse/RHDHPAI-1064
This PR introduces
PUT /conversations/{conversation_id}
to allow topic summary updatewith this endpoint, user can update a conversation's topic as wish, so that it's more easier for them to organize & manage the conversations.
Tested with the conversation, with initial topic summary

after a a PUT request


also tested with non-exist conversation:

Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Tests