LCORE-793: Refactored status codes and response models for v1/conversations #683
Conversation
WalkthroughReplace flat string error responses with nested DetailModel-based error schemas; add retrieve_conversation and can_access_conversation utilities; update GET/DELETE conversation handlers, OpenAPI, and tests to use new response models and adjusted status codes. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant E as Endpoint (/v1/conversations/{id})
participant U as Utils (can_access_conversation / retrieve_conversation)
participant DB as Database
C->>E: GET or DELETE /v1/conversations/{id}
E->>U: can_access_conversation(id, user_id, others_allowed)
U->>DB: query owner by conversation id
DB-->>U: owner or none
alt access denied
U-->>E: False
Note over E,C #fff0f0: return AccessDeniedResponse (403)
E-->>C: 403 AccessDeniedResponse
else access allowed
U-->>E: True
E->>U: retrieve_conversation(id)
U->>DB: fetch conversation record
DB-->>U: conversation or none
alt not found
U-->>E: None
Note over E,C #fff8e6: return NotFoundResponse (404)
E-->>C: 404 NotFoundResponse
else found
U-->>E: conversation
Note over E,C #eef7ee: return ConversationResponse (200) or deletion result
E-->>C: 200 ConversationResponse / 200/204 delete
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/endpoints.py (1)
371-401: Return order mismatch and docstring inconsistency in get_temp_agentget_temp_agent returns (agent, session_id, conversation_id) but get_agent returns (agent, conversation_id, session_id). This inconsistency will cause subtle bugs.
Align order and fix docstring:
async def get_temp_agent( client: AsyncLlamaStackClient, model_id: str, system_prompt: str, ) -> tuple[AsyncAgent, str, str]: - """Create a temporary agent with new agent_id and session_id. + """Create a temporary agent with new agent_id and session_id. This function creates a new agent without persistence, shields, or tools. Useful for temporary operations or one-off queries, such as validating a question or generating a summary. Args: client: The AsyncLlamaStackClient to use for the request. model_id: The ID of the model to use. system_prompt: The system prompt/instructions for the agent. Returns: - tuple[AsyncAgent, str]: A tuple containing the agent and session_id. + tuple[AsyncAgent, str, str]: (agent, conversation_id, session_id). """ @@ - return agent, session_id, conversation_id + return agent, conversation_id, session_id
🧹 Nitpick comments (5)
src/utils/endpoints.py (1)
88-117: Harden owner lookup and avoid double DB hits
- Querying a single column then accessing row.user_id can be brittle across SQLAlchemy versions; prefer scalar() to get the value directly.
- Also consider returning the conversation (or owner id) once and reusing it to avoid the subsequent retrieve_conversation call.
Apply for robustness:
- with get_session() as session: - row = ( - session.query(UserConversation.user_id) - .filter(UserConversation.id == conversation_id) - .first() - ) - - # If conversation does not exist, permissions check returns True - if row is None: - return True - - # If conversation exists, user_id must match - return row.user_id == user_id + with get_session() as session: + owner_user_id = ( + session.query(UserConversation.user_id) + .filter(UserConversation.id == conversation_id) + .scalar() + ) + # If conversation does not exist, return True so callers can emit 404 + if owner_user_id is None: + return True + return owner_user_id == user_idsrc/models/responses.py (1)
958-1117: Structured error models look solid; consider simplifying 403 variants
- DetailModel + AbstractErrorResponse with dump_detail() is a good pattern.
- You now have both AccessDeniedResponse and ForbiddenResponse producing 403-like payloads; consider consolidating to a single class to avoid confusion.
- BadRequestResponse currently hardcodes “Invalid conversation ID format”; if reused for other resources, you may want a generic message based on resource.
src/app/endpoints/conversations.py (3)
35-35: Use module logger per guidelinesPrefer logger = logging.getLogger(name) to follow logging best practices.
As per coding guidelines
-logger = logging.getLogger("app.endpoints.handlers") +logger = logging.getLogger(__name__)
108-116: Tighten type hints for session_dataSpecify dict[str, Any] for clarity.
As per coding guidelines
-def simplify_session_data(session_data: dict) -> list[dict[str, Any]]: +def simplify_session_data(session_data: dict[str, Any]) -> list[dict[str, Any]]:
131-138: Avoid appending empty output messagesOnly append output if present; prevents None entries.
- output_msg = turn.get("output_message", {}) - cleaned_messages.append( - { - "content": output_msg.get("content"), - "type": output_msg.get("role"), # Rename role to type - } - ) + output_msg = turn.get("output_message") + if output_msg: + cleaned_messages.append( + { + "content": output_msg.get("content"), + "type": output_msg.get("role"), # Rename role to type + } + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/openapi.json(13 hunks)src/app/endpoints/conversations.py(7 hunks)src/models/responses.py(2 hunks)src/utils/endpoints.py(2 hunks)tests/e2e/features/conversations.feature(2 hunks)tests/e2e/features/query.feature(1 hunks)tests/integration/test_openapi_json.py(1 hunks)tests/unit/app/endpoints/test_conversations.py(15 hunks)tests/unit/models/responses/test_unauthorized_response.py(1 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.pysrc/utils/endpoints.pytests/unit/models/responses/test_unauthorized_response.pytests/unit/app/endpoints/test_conversations.pysrc/models/responses.pysrc/app/endpoints/conversations.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.pytests/unit/models/responses/test_unauthorized_response.pytests/unit/app/endpoints/test_conversations.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.pytests/unit/models/responses/test_unauthorized_response.pytests/unit/app/endpoints/test_conversations.py
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Write E2E tests as Gherkin feature files for behave
Files:
tests/e2e/features/conversations.featuretests/e2e/features/query.feature
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/utils/endpoints.pysrc/models/responses.pysrc/app/endpoints/conversations.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/**/*.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/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.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.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.py
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/app/endpoints/**/*.py : In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Applied to files:
src/models/responses.py
🧬 Code graph analysis (4)
src/utils/endpoints.py (2)
src/models/database/conversations.py (1)
UserConversation(11-38)src/app/database.py (1)
get_session(34-40)
tests/unit/models/responses/test_unauthorized_response.py (1)
src/models/responses.py (2)
UnauthorizedResponse(1073-1094)DetailModel(958-962)
tests/unit/app/endpoints/test_conversations.py (4)
src/models/database/conversations.py (1)
UserConversation(11-38)tests/unit/app/endpoints/test_query.py (1)
dummy_request(53-62)src/models/config.py (1)
Action(328-374)src/app/endpoints/conversations.py (2)
get_conversation_endpoint_handler(214-341)delete_conversation_endpoint_handler(348-468)
src/app/endpoints/conversations.py (4)
src/models/responses.py (9)
NotFoundResponse(1026-1047)AccessDeniedResponse(1002-1023)BadRequestResponse(978-999)ServiceUnavailableResponse(1050-1070)ConversationResponse(600-665)UnauthorizedResponse(1073-1094)ConversationDeleteResponse(668-701)ConversationsListResponse(773-834)dump_detail(973-975)src/utils/endpoints.py (4)
check_configuration_loaded(119-131)delete_conversation(28-42)can_access_conversation(88-116)retrieve_conversation(45-62)src/utils/suid.py (1)
check_suid(19-38)src/models/config.py (1)
Action(328-374)
🪛 GitHub Actions: Black
src/app/endpoints/conversations.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted by Black. Run 'uv tool run black --check .' to verify, or 'uv tool run black .' to format.
🪛 GitHub Actions: Python linter
src/app/endpoints/conversations.py
[error] 237-237: pylint: Trailing whitespace (trailing-whitespace). Command: uv run pylint src tests
⏰ 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). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (14)
src/utils/endpoints.py (1)
45-63: Helper looks goodRetrieval scoped and typed; filters by id and user_id correctly.
tests/integration/test_openapi_json.py (1)
69-69: OpenAPI expectations updated appropriatelyRemoving 400 from GET /v1/conversations aligns with list semantics.
tests/e2e/features/query.feature (1)
71-72: E2E expectation aligned to 404 Not FoundSwitch to 404 with "Conversation not found" matches new error model.
tests/e2e/features/conversations.feature (3)
117-117: Use 400 for malformed conversation_idSwitching GET .../{conversation_id} malformed ID to 400 matches BadRequest semantics.
156-156: DELETE malformed conversation_id → 400Consistent with GET and new error model.
160-160: Auth header added for delete-not-found scenarioGood addition to exercise auth-dependent paths.
tests/unit/models/responses/test_unauthorized_response.py (1)
3-21: Tests correctly validate DetailModel-based UnauthorizedResponseCovers default and user-specific causes; looks good.
tests/unit/app/endpoints/test_conversations.py (1)
182-193: Fixture for mock_conversation is fineUsing a lightweight instance with assigned attributes suffices for tests.
src/app/endpoints/conversations.py (2)
237-238: Fix formatting: trailing whitespace causes CI failuresPylint trailing-whitespace on Line 237; Black also wants to reformat. Remove the whitespace-only line and run Black.
Apply:
- +Then run:
- uv tool run black .
- uv run pylint src tests
40-62: Responses mapping alignment looks good200/400/401/403/404/503 now reference unified models with precise descriptions.
Also applies to: 67-90, 94-105
docs/openapi.json (4)
696-704: Docs/runtime mismatch: 500 vs 503 for conversations listCode can raise 500 on unknown errors, but OpenAPI lists 503 only. Align by either:
- Adding 500 to this endpoint’s responses, or
- Mapping unknown exceptions to 503 in code.
Also applies to: 705-714, 715-724
749-756: Docs/runtime mismatch: 500 not documented for GET conversationget_conversation_endpoint_handler raises 500 on unknown errors; spec lists 503 but not 500. Add 500 to responses or convert the unknown error path to 503.
Also applies to: 759-767, 779-787, 789-797, 799-807
840-848: Docs/runtime mismatch: 500 not documented for DELETE conversationdelete_conversation_endpoint_handler raises 500 on unknown errors; spec lists 503 but not 500. Add 500 to responses or convert unknown errors to 503.
Also applies to: 850-858, 870-878, 880-888, 890-898
1313-1331: Unified error schemas look solidDetailModel + AccessDenied/BadRequest/NotFound/ServiceUnavailable/Unauthorized responses are well-structured and consistent.
Also applies to: 1618-1636, 2312-2332, 2979-2997, 3759-3777, 3965-3979
asimurka
force-pushed
the
v1/conversations_status_codes_refactor
branch
from
8f6a0e8 to
642cff0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/unit/app/endpoints/test_conversations.py (2)
399-442: 403 test won’t trigger without restricting request permissionsHandler reads request.state.authorized_actions, not NoopAccessResolver.get_actions. Limit authorized_actions to own-only to exercise 403. This was flagged earlier.
@@ - mocker.patch( - "authorization.resolvers.NoopAccessResolver.get_actions", - return_value=set(Action.GET_CONVERSATION), - ) # Reduce user's permissions to access only their conversations + # Restrict request permissions to own conversations only + dummy_request.state.authorized_actions = {Action.GET_CONVERSATION}
666-709: DELETE 403 test has the same permission issueSet authorized_actions to own-delete only so 403 is exercised. Previously noted as well.
@@ - mocker.patch( - "authorization.resolvers.NoopAccessResolver.get_actions", - return_value=set(Action.DELETE_CONVERSATION), - ) # Reduce user's permissions to delete only their conversations + # Restrict permissions to deleting only own conversations + dummy_request.state.authorized_actions = {Action.DELETE_CONVERSATION}src/app/endpoints/conversations.py (2)
270-277: Critical: Authorized cross-user access still returns 404This critical issue was flagged in previous reviews but remains unresolved. When
READ_OTHERS_CONVERSATIONSpermission is granted,can_access_conversationreturnsTrue, butretrieve_conversation(conversation_id, user_id)filters byuser_id, causing a404for non-owned conversations.Solution: Conditionally retrieve by ID only when others access is permitted:
- conversation = retrieve_conversation(conversation_id, user_id) + # Retrieve conversation; allow cross-user fetch if authorized + if Action.READ_OTHERS_CONVERSATIONS in request.state.authorized_actions: + with get_session() as session: + conversation = ( + session.query(UserConversation) + .filter_by(id=conversation_id) + .first() + ) + else: + conversation = retrieve_conversation(conversation_id, user_id) if conversation is None: raise HTTPException(
399-406: Critical: Authorized cross-user delete still returns 404This critical issue was flagged in previous reviews but remains unresolved. When
DELETE_OTHERS_CONVERSATIONSpermission is granted,can_access_conversationreturnsTrue, butretrieve_conversation(conversation_id, user_id)filters byuser_id, causing a404for non-owned conversations.Solution: Conditionally retrieve by ID only when others access is permitted:
- conversation = retrieve_conversation(conversation_id, user_id) + # Retrieve conversation; allow cross-user fetch if authorized + if Action.DELETE_OTHERS_CONVERSATIONS in request.state.authorized_actions: + with get_session() as session: + conversation = ( + session.query(UserConversation) + .filter_by(id=conversation_id) + .first() + ) + else: + conversation = retrieve_conversation(conversation_id, user_id) if conversation is None: raise HTTPException(
🧹 Nitpick comments (7)
src/models/responses.py (3)
1076-1094: Unify union/Optional style across this moduleNew code uses str | None while older parts use Optional[T]. Pick one and apply consistently for readability.
As per coding guidelines
1097-1106: Avoid inheriting ForbiddenResponse from UnauthorizedResponse; derive from AbstractErrorResponseInheritance here is semantically odd and requires post-init mutation. Derive directly and construct detail via super().init.
-class ForbiddenResponse(UnauthorizedResponse): +class ForbiddenResponse(AbstractErrorResponse): @@ - def __init__(self, user_id: str, resource: str, resource_id: str): - """Initialize a ForbiddenResponse when user is authenticated but lacks resource access.""" - super().__init__(user_id=user_id) - self.detail = DetailModel( - response="Access denied", - cause=f"User {user_id} is not allowed to access {resource} with ID {resource_id}.", - ) + def __init__(self, user_id: str, resource: str, resource_id: str): + """Initialize a ForbiddenResponse when user is authenticated but lacks resource access.""" + super().__init__( + detail=DetailModel( + response="Access denied", + cause=f"User {user_id} is not allowed to access {resource} with ID {resource_id}.", + ) + )
1005-1011: Minimize PII in error messagesReturning user_id in 403 causes may be unnecessary. Consider generic phrasing without echoing identifiers to clients.
Also applies to: 1104-1106
docs/openapi.json (1)
1313-1331: Consider a single 403 schemaAccessDeniedResponse and ForbiddenResponse duplicate semantics. Prefer one 403 schema to simplify client handling.
Also applies to: 2574-2591
tests/unit/app/endpoints/test_conversations.py (2)
444-456: Redundant patching of get_actionsThe handler ignores resolver.get_actions; it uses request.state.authorized_actions. Either set that explicitly or drop this patch.
710-721: Minor: allowed-delete test doesn’t need resolver patchEither set request.state.authorized_actions to include DELETE_OTHERS_CONVERSATIONS or remove this no-op patch.
src/app/endpoints/conversations.py (1)
337-340: Consider using a response model for 500 errorsWhile other errors now use structured response models, the 500 error still uses an inline dict format. For consistency, consider creating an
InternalServerErrorResponsemodel.Example:
# In src/models/responses.py class InternalServerErrorResponse(AbstractErrorResponse): def __init__(self, cause: str): super().__init__( detail=DetailModel( response="Unknown error", cause=cause ) )Then use:
raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail={ - "response": "Unknown error", - "cause": f"Unknown error while getting conversation {conversation_id} : {str(e)}", - }, + detail=InternalServerErrorResponse( + cause=f"Unknown error while getting conversation {conversation_id} : {str(e)}" + ).dump_detail(), ) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/openapi.json(13 hunks)src/app/endpoints/conversations.py(7 hunks)src/models/responses.py(2 hunks)src/utils/endpoints.py(2 hunks)tests/e2e/features/conversations.feature(2 hunks)tests/e2e/features/query.feature(1 hunks)tests/integration/test_openapi_json.py(1 hunks)tests/unit/app/endpoints/test_conversations.py(15 hunks)tests/unit/models/responses/test_unauthorized_response.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/endpoints.py
- tests/e2e/features/query.feature
- tests/integration/test_openapi_json.py
🧰 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/unit/models/responses/test_unauthorized_response.pysrc/app/endpoints/conversations.pysrc/models/responses.pytests/unit/app/endpoints/test_conversations.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/models/responses/test_unauthorized_response.pytests/unit/app/endpoints/test_conversations.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/models/responses/test_unauthorized_response.pytests/unit/app/endpoints/test_conversations.py
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.pysrc/models/responses.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.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.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.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/**/*.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
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Write E2E tests as Gherkin feature files for behave
Files:
tests/e2e/features/conversations.feature
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/app/endpoints/**/*.py : In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Applied to files:
src/models/responses.py
🧬 Code graph analysis (3)
tests/unit/models/responses/test_unauthorized_response.py (1)
src/models/responses.py (2)
UnauthorizedResponse(1073-1094)DetailModel(958-962)
src/app/endpoints/conversations.py (3)
src/models/responses.py (9)
NotFoundResponse(1026-1047)AccessDeniedResponse(1002-1023)BadRequestResponse(978-999)ServiceUnavailableResponse(1050-1070)ConversationResponse(600-665)UnauthorizedResponse(1073-1094)ConversationDeleteResponse(668-701)ConversationsListResponse(773-834)dump_detail(973-975)src/utils/endpoints.py (4)
check_configuration_loaded(119-131)delete_conversation(28-42)can_access_conversation(88-116)retrieve_conversation(45-62)src/models/config.py (1)
Action(328-374)
tests/unit/app/endpoints/test_conversations.py (4)
src/models/database/conversations.py (1)
UserConversation(11-38)tests/unit/app/endpoints/test_query.py (1)
dummy_request(53-62)src/models/config.py (1)
Action(328-374)src/app/endpoints/conversations.py (2)
get_conversation_endpoint_handler(214-341)delete_conversation_endpoint_handler(348-468)
⏰ 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). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (14)
src/models/responses.py (1)
958-976: Structured error model + dump_detail are solidNice abstraction; nested DetailModel + dump_detail aligns with FastAPI HTTPException payloads.
tests/e2e/features/conversations.feature (2)
117-117: Correct: 400 for malformed conversation_idMatches new BadRequestResponse behavior.
160-160: Good addition: set Authorization for non-existent DELETE casePrevents unauthorized short-circuit.
tests/unit/models/responses/test_unauthorized_response.py (1)
9-14: LGTM: tests accurately cover new UnauthorizedResponse semanticsCovers default and user-scoped causes with DetailModel.
Also applies to: 16-21
docs/openapi.json (4)
696-704: Conversations list: responses standardized200/401/503 now reference concrete schemas. Ensure tests align with 401 for missing/invalid token (e2e still asserts 400).
Also applies to: 705-721, 715-724
749-756: Conversations GET by ID: correct 400/403/404/503 schemasGood alignment with new DetailModel-based errors.
Also applies to: 759-767, 779-787, 789-797, 799-807
840-848: Conversations DELETE by ID: correct 200/400/403/404/503 schemasConsistent with endpoint behavior and tests.
Also applies to: 850-858, 869-878, 880-888, 890-898
1313-1331: New error component schemas look goodDetailModel and concrete error wrappers are consistent and reusable.
Also applies to: 1618-1636, 2312-2332, 2979-2997, 3759-3777, 3965-3979
tests/unit/app/endpoints/test_conversations.py (2)
16-16: Importing UserConversation is fineFixture uses it to build realistic objects.
182-193: Fixture OKCreates a minimal UserConversation for tests.
src/app/endpoints/conversations.py (4)
38-105: LGTM: Response dictionaries properly updatedThe response dictionaries correctly reference the new response models (
ConversationResponse,ConversationDeleteResponse,ConversationsListResponse,BadRequestResponse,AccessDeniedResponse,NotFoundResponse,ServiceUnavailableResponse) with appropriate descriptions for each status code.
239-246: LGTM: Proper validation and error responseThe conversation ID validation correctly uses
BadRequestResponsewith the structured detail format.
282-327: LGTM: Proper error handling with new response modelsThe error handling correctly uses
NotFoundResponsefor missing sessions and not-found errors, andServiceUnavailableResponsefor API connection errors. The re-raise pattern forHTTPExceptionis appropriate.
440-455: LGTM: Proper error handling with new response modelsThe error handling correctly uses
ServiceUnavailableResponsefor API connection errors andNotFoundResponsefor not-found errors. The re-raise pattern forHTTPExceptionis appropriate.
asimurka
force-pushed
the
v1/conversations_status_codes_refactor
branch
2 times, most recently
from
f9ec96b to
5ae06bd
Compare
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)
tests/e2e/features/conversations.feature (1)
39-45: Update missing-auth expectations in E2E tests to 401 with structured DetailModel response.The codebase defines
UnauthorizedResponseandDetailModel(lines 1073–1083 insrc/models/responses.py), and endpoints document both 400 and 401 mapped toUnauthorizedResponsewith structured detail. Unit tests confirm the expected format:{"detail": {"response": "Unauthorized", "cause": "Missing or invalid credentials provided by client"}}(test_unauthorized_response.py:11–21).Lines 39–45 and 105–111 in
tests/e2e/features/conversations.featurecurrently assert status 400 with a flat string detail, but should assert:
- Status: 401 (matching the design; see conversations_v2.py:48–50)
- Body:
{"detail": {"response": "Unauthorized", "cause": "Missing or invalid credentials provided by client"}}Also applies to lines 265, 58–59 in
tests/e2e/features/feedback.featureandtests/e2e/features/authorized_noop_token.feature, and line 61 intests/e2e/features/query.feature.
♻️ Duplicate comments (5)
tests/e2e/features/conversations.feature (1)
174-176: Use DELETE, not GET, in “DELETE … llama-stack is unavailable” scenario.This scenario must exercise DELETE. Apply:
- When I use REST API conversation endpoint with conversation_id from above using HTTP GET method + When I use REST API conversation endpoint with conversation_id from above using HTTP DELETE methodtests/unit/app/endpoints/test_conversations.py (4)
399-413: Forbidden (GET) test does not restrict actions; mock resolvers, not NoopAccessResolver.get_actions.The middleware overwrites request.state.authorized_actions. Patch get_authorization_resolvers to return limited actions:
- mocker.patch( - "authorization.resolvers.NoopAccessResolver.get_actions", - return_value=set(Action.GET_CONVERSATION), - ) # Reduce user's permissions to access only their conversations + # Limit actions to own conversations only + mock_resolvers = mocker.patch("authorization.middleware.get_authorization_resolvers") + mock_role_resolver = mocker.AsyncMock() + mock_access_resolver = mocker.Mock() + mock_role_resolver.resolve_roles.return_value = set() + mock_access_resolver.check_access.return_value = True + mock_access_resolver.get_actions.return_value = {Action.GET_CONVERSATION} + mock_resolvers.return_value = (mock_role_resolver, mock_access_resolver)
454-456: Allow-read-others (GET) test should also mock resolvers, not NoopAccessResolver.get_actions.Apply resolver-based mocking to include READ_OTHERS_CONVERSATIONS:
- mocker.patch( - "authorization.resolvers.NoopAccessResolver.get_actions", - return_value={Action.GET_CONVERSATION, Action.READ_OTHERS_CONVERSATIONS}, - ) # Allow user to access other users' conversations + mock_resolvers = mocker.patch("authorization.middleware.get_authorization_resolvers") + mock_role_resolver = mocker.AsyncMock() + mock_access_resolver = mocker.Mock() + mock_role_resolver.resolve_roles.return_value = set() + mock_access_resolver.check_access.return_value = True + mock_access_resolver.get_actions.return_value = { + Action.GET_CONVERSATION, Action.READ_OTHERS_CONVERSATIONS + } + mock_resolvers.return_value = (mock_role_resolver, mock_access_resolver)
678-680: Forbidden (DELETE) test needs resolver mocking to restrict actions.Use the middleware resolver patch to exclude DELETE_OTHERS_CONVERSATIONS:
- mocker.patch( - "authorization.resolvers.NoopAccessResolver.get_actions", - return_value=set(Action.DELETE_CONVERSATION), - ) # Reduce user's permissions to delete only their conversations + mock_resolvers = mocker.patch("authorization.middleware.get_authorization_resolvers") + mock_role_resolver = mocker.AsyncMock() + mock_access_resolver = mocker.Mock() + mock_role_resolver.resolve_roles.return_value = set() + mock_access_resolver.check_access.return_value = True + mock_access_resolver.get_actions.return_value = {Action.DELETE_CONVERSATION} + mock_resolvers.return_value = (mock_role_resolver, mock_access_resolver)
716-721: Allow-delete-others (DELETE) test should mock resolvers to include DELETE_OTHERS_CONVERSATIONS.Apply resolver-based mocking:
- mocker.patch( - "authorization.resolvers.NoopAccessResolver.get_actions", - return_value={ - Action.DELETE_OTHERS_CONVERSATIONS, - Action.DELETE_CONVERSATION, - }, - ) # Allow user to detele other users' conversations + mock_resolvers = mocker.patch("authorization.middleware.get_authorization_resolvers") + mock_role_resolver = mocker.AsyncMock() + mock_access_resolver = mocker.Mock() + mock_role_resolver.resolve_roles.return_value = set() + mock_access_resolver.check_access.return_value = True + mock_access_resolver.get_actions.return_value = { + Action.DELETE_OTHERS_CONVERSATIONS, Action.DELETE_CONVERSATION + } + mock_resolvers.return_value = (mock_role_resolver, mock_access_resolver)
🧹 Nitpick comments (8)
docs/openapi.json (1)
1619-1636: New error schemas wired to DetailModel look solid.Consistent structure across BadRequest/NotFound/ServiceUnavailable/Unauthorized/AccessDenied/Forbidden.
Consider unifying naming (AccessDenied vs Forbidden) to one across endpoints to reduce client confusion.
Also applies to: 2980-2997, 3760-3777, 2313-2332, 3965-3980, 1313-1331, 2576-2591
tests/unit/app/endpoints/test_conversations.py (1)
312-314: Set explicit return_value on patched helpers to avoid truthiness ambiguity.Make intent clear and robust:
-mocker.patch("app.endpoints.conversations.can_access_conversation") +mocker.patch("app.endpoints.conversations.can_access_conversation", return_value=True) -mocker.patch("app.endpoints.conversations.retrieve_conversation") +mocker.patch("app.endpoints.conversations.retrieve_conversation", return_value=object())Also applies to: 342-344, 375-377, 499-501, 577-579, 607-608, 639-641, 761-763
src/models/responses.py (3)
981-988: Generalize BadRequestResponse message to any resourceThe message is hard-coded to “conversation”. Use the provided resource parameter for consistency.
- super().__init__( - detail=DetailModel( - response="Invalid conversation ID format", - cause=f"{resource.title()} ID {resource_id} has invalid format", - ) - ) + super().__init__( + detail=DetailModel( + response=f"Invalid {resource} ID format", + cause=f"{resource.title()} ID {resource_id} has invalid format", + ) + )
1097-1107: Prefer inheriting from AbstractErrorResponse for 403 ForbiddenSubclassing UnauthorizedResponse and then mutating detail is confusing. Inherit from AbstractErrorResponse and set detail via super for clarity.
-class ForbiddenResponse(UnauthorizedResponse): +class ForbiddenResponse(AbstractErrorResponse): @@ - def __init__(self, user_id: str, resource: str, resource_id: str): - """Initialize a ForbiddenResponse when user is authenticated but lacks resource access.""" - super().__init__(user_id=user_id) - self.detail = DetailModel( - response="Access denied", - cause=f"User {user_id} is not allowed to access {resource} with ID {resource_id}.", - ) + def __init__(self, user_id: str, resource: str, resource_id: str): + """Initialize a ForbiddenResponse when user is authenticated but lacks resource access.""" + super().__init__( + detail=DetailModel( + response="Access denied", + cause=f"User {user_id} is not allowed to access {resource} with ID {resource_id}.", + ) + )
1073-1095: Optional: Confirm if ForbiddenResponse is usedAccessDeniedResponse is used for 403s in endpoints. If ForbiddenResponse is unused, consider removing or aligning usage for one canonical 403 model to reduce surface area.
Would you like me to scan the repo for ForbiddenResponse usages and propose cleanup?
src/app/endpoints/conversations.py (3)
334-343: Unify 500 error payloads with new nested detail model and document 500These two 500s return a raw dict, diverging from the new DetailModel-based pattern and aren’t declared in responses. Add a 500 response model and use it here.
Apply in this file:
@@ from models.responses import ( - ServiceUnavailableResponse, + ServiceUnavailableResponse, + InternalServerErrorResponse, ) @@ conversation_responses = { - 503: { + 503: { "model": ServiceUnavailableResponse, "description": "Service unavailable", }, + 500: { + "model": InternalServerErrorResponse, + "description": "Internal server error", + }, } @@ conversation_delete_responses = { - 503: { + 503: { "model": ServiceUnavailableResponse, "description": "Service unavailable", }, + 500: { + "model": InternalServerErrorResponse, + "description": "Internal server error", + }, } @@ conversations_list_responses = { - 503: { + 503: { "model": ServiceUnavailableResponse, "description": "Service unavailable", }, + 500: { + "model": InternalServerErrorResponse, + "description": "Internal server error", + }, } @@ except Exception as e: - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail={ - "response": "Unknown error", - "cause": f"Unknown error while getting conversation {conversation_id} : {str(e)}", - }, - ) from e + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=InternalServerErrorResponse( + cause=f"Unknown error while getting conversation {conversation_id} : {str(e)}" + ).dump_detail(), + ) from e @@ except Exception as e: - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail={ - "response": "Unknown error", - "cause": f"Unknown error while deleting conversation {conversation_id} : {str(e)}", - }, - ) from e + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=InternalServerErrorResponse( + cause=f"Unknown error while deleting conversation {conversation_id} : {str(e)}" + ).dump_detail(), + ) from eAdd this model to src/models/responses.py:
class InternalServerErrorResponse(AbstractErrorResponse): """500 Internal Server Error - Unexpected failure.""" def __init__(self, cause: str): super().__init__( detail=DetailModel( response="Unknown error", cause=cause, ) ) model_config = { "json_schema_extra": { "examples": [{ "detail": { "response": "Unknown error", "cause": "Unexpected exception while processing the request", } }] } }Also applies to: 462-471
256-260: PII in logs: review policy for logging user/conversation IDsThese warning messages log user_id and conversation_id. If IDs are considered PII in your environment, mask or downgrade to debug per policy.
Also applies to: 386-390
271-271: Typo: “retreive” → “retrieve”Minor comment fix.
- # If reached this, user is authorized to retreive this conversation + # If reached this, user is authorized to retrieve this conversation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/openapi.json(13 hunks)src/app/endpoints/conversations.py(7 hunks)src/models/responses.py(2 hunks)src/utils/endpoints.py(2 hunks)tests/e2e/features/conversations.feature(2 hunks)tests/e2e/features/query.feature(1 hunks)tests/integration/test_openapi_json.py(1 hunks)tests/unit/app/endpoints/test_conversations.py(15 hunks)tests/unit/models/responses/test_unauthorized_response.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/features/query.feature
- tests/integration/test_openapi_json.py
🧰 Additional context used
📓 Path-based instructions (10)
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Write E2E tests as Gherkin feature files for behave
Files:
tests/e2e/features/conversations.feature
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/utils/endpoints.pysrc/app/endpoints/conversations.pysrc/models/responses.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/utils/endpoints.pytests/unit/models/responses/test_unauthorized_response.pysrc/app/endpoints/conversations.pysrc/models/responses.pytests/unit/app/endpoints/test_conversations.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/models/responses/test_unauthorized_response.pytests/unit/app/endpoints/test_conversations.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/models/responses/test_unauthorized_response.pytests/unit/app/endpoints/test_conversations.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.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.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.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/**/*.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
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/app/endpoints/**/*.py : In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Applied to files:
src/models/responses.py
🧬 Code graph analysis (4)
src/utils/endpoints.py (2)
src/models/database/conversations.py (1)
UserConversation(11-38)src/app/database.py (1)
get_session(34-40)
tests/unit/models/responses/test_unauthorized_response.py (1)
src/models/responses.py (2)
UnauthorizedResponse(1073-1094)DetailModel(958-962)
src/app/endpoints/conversations.py (3)
src/models/responses.py (9)
NotFoundResponse(1026-1047)AccessDeniedResponse(1002-1023)BadRequestResponse(978-999)ServiceUnavailableResponse(1050-1070)ConversationResponse(600-665)UnauthorizedResponse(1073-1094)ConversationDeleteResponse(668-701)ConversationsListResponse(773-834)dump_detail(973-975)src/utils/endpoints.py (3)
delete_conversation(28-42)can_access_conversation(81-108)retrieve_conversation(45-55)src/models/config.py (1)
Action(328-374)
tests/unit/app/endpoints/test_conversations.py (3)
src/models/database/conversations.py (1)
UserConversation(11-38)src/models/config.py (1)
Action(328-374)src/app/endpoints/conversations.py (2)
get_conversation_endpoint_handler(214-343)delete_conversation_endpoint_handler(350-471)
⏰ 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: e2e_tests (azure)
🔇 Additional comments (10)
tests/e2e/features/conversations.feature (2)
117-117: LGTM: 400 for malformed conversation_id (GET).This matches the BadRequestResponse mapping.
156-156: LGTM: 400 for malformed conversation_id (DELETE).Aligned with BadRequestResponse in v1 delete.
tests/unit/models/responses/test_unauthorized_response.py (1)
3-21: LGTM: tests reflect DetailModel-based UnauthorizedResponse.Assertions correctly validate response and cause for both with/without user_id.
src/utils/endpoints.py (2)
45-56: LGTM: retrieve_conversation helper.Simple, typed, and uses session context correctly.
81-109: LGTM: access check with no-enumeration behavior.Early allow for others_allowed and “not found returns True” avoids ID enumeration; downstream 404 handles absence.
Please ensure unit tests cover both branches: (a) others_allowed True, and (b) non-existent conversation -> subsequent 404 from retrieve.
docs/openapi.json (3)
696-704: Spec aligns for GET /v1/conversations (200/401/503).Ensure E2E tests expect 401 with DetailModel for missing auth, not 400 with flat string.
Also applies to: 705-714, 715-724
749-757: GET /v1/conversations/{id}: responses standardized.200/400/401/403/404/503 mappings align with new models.
Also applies to: 759-767, 779-787, 789-797, 799-807
840-848: DELETE /v1/conversations/{id}: responses standardized.200/400/401/403/404/503 mappings align with new models.
Also applies to: 850-858, 869-878, 880-888, 890-898
tests/unit/app/endpoints/test_conversations.py (1)
182-193: LGTM: mock_conversation fixture.Sufficient fields for access tests; consistent with UserConversation model.
src/app/endpoints/conversations.py (1)
38-63: LGTM on response mappings and models for 200/400/401/403/404/503Mappings and model choices align with the refactor goals; docs should reflect the new DetailModel-based schemas.
Please ensure the OpenAPI responses for these routes were regenerated and validated in Swagger UI.
Also applies to: 65-90, 92-105
asimurka
force-pushed
the
v1/conversations_status_codes_refactor
branch
from
5ae06bd to
f25bb71
Compare
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)
tests/e2e/features/conversations.feature (1)
174-175: Fix HTTP method in “DELETE … llama-stack is unavailable” scenarioThe step still uses HTTP GET; it should be HTTP DELETE to exercise the DELETE path.
- When I use REST API conversation endpoint with conversation_id from above using HTTP GET method + When I use REST API conversation endpoint with conversation_id from above using HTTP DELETE method
🧹 Nitpick comments (5)
tests/unit/app/endpoints/test_conversations.py (1)
312-314: Make mocked returns explicit for claritySet explicit return_value to avoid relying on Mock truthiness and to self-document intent.
- mocker.patch("app.endpoints.conversations.can_access_conversation") - mocker.patch("app.endpoints.conversations.retrieve_conversation") + mocker.patch("app.endpoints.conversations.can_access_conversation", return_value=True) + mocker.patch("app.endpoints.conversations.retrieve_conversation", return_value=mocker.Mock()) # Same adjustment in other tests patching these helpers.Also applies to: 342-344, 375-377, 499-501, 577-579, 606-608, 639-641, 761-763
src/models/responses.py (2)
979-989: Generalize BadRequestResponse messageHardcoded “Invalid conversation ID format” ignores the resource argument. Use resource-specific wording.
- detail=DetailModel( - response="Invalid conversation ID format", - cause=f"{resource.title()} ID {resource_id} has invalid format", - ) + detail=DetailModel( + response=f"Invalid {resource} ID format", + cause=f"{resource.title()} ID {resource_id} has invalid format", + )
973-976: Minor: add precise return type to dump_detailHelps consumers and IDEs.
- def dump_detail(self) -> dict: + def dump_detail(self) -> dict[str, str]:src/app/endpoints/conversations.py (2)
271-271: Fix typo in comments.The word "retreive" should be "retrieve" in both comments.
Apply this diff:
- # If reached this, user is authorized to retreive this conversation + # If reached this, user is authorized to retrieve this conversationAnd at line 401:
- # If reached this, user is authorized to retreive this conversation + # If reached this, user is authorized to retrieve this conversationAlso applies to: 401-401
205-209: Consider using response models for 500 errors for consistency.The 500 Internal Server Error responses at lines 205-209, 339-342, and 467-470 still use inline dictionaries, while all other error responses use the new structured response models (e.g.,
BadRequestResponse,NotFoundResponse, etc.).For consistency and maintainability, consider creating an
InternalServerErrorResponsemodel similar to the other error response models and using it here.Also applies to: 339-342, 467-470
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/openapi.json(13 hunks)src/app/endpoints/conversations.py(7 hunks)src/models/responses.py(2 hunks)src/utils/endpoints.py(2 hunks)tests/e2e/features/conversations.feature(2 hunks)tests/e2e/features/query.feature(1 hunks)tests/integration/test_openapi_json.py(1 hunks)tests/unit/app/endpoints/test_conversations.py(15 hunks)tests/unit/models/responses/test_unauthorized_response.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/endpoints.py
- tests/integration/test_openapi_json.py
🧰 Additional context used
📓 Path-based instructions (10)
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.pysrc/models/responses.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.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.pysrc/models/responses.pytests/unit/app/endpoints/test_conversations.pytests/unit/models/responses/test_unauthorized_response.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.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.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/**/*.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
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.pytests/unit/models/responses/test_unauthorized_response.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.pytests/unit/models/responses/test_unauthorized_response.py
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Write E2E tests as Gherkin feature files for behave
Files:
tests/e2e/features/query.featuretests/e2e/features/conversations.feature
🧬 Code graph analysis (3)
src/app/endpoints/conversations.py (3)
src/models/responses.py (9)
NotFoundResponse(1030-1053)AccessDeniedResponse(1004-1027)BadRequestResponse(978-1001)ServiceUnavailableResponse(1056-1078)ConversationResponse(600-665)UnauthorizedResponse(1081-1104)ConversationDeleteResponse(668-701)ConversationsListResponse(773-834)dump_detail(973-975)src/utils/endpoints.py (4)
check_configuration_loaded(111-123)delete_conversation(28-42)can_access_conversation(81-108)retrieve_conversation(45-55)src/models/config.py (1)
Action(328-374)
tests/unit/app/endpoints/test_conversations.py (4)
src/models/database/conversations.py (1)
UserConversation(11-38)tests/unit/app/endpoints/test_query.py (1)
dummy_request(53-62)src/models/config.py (1)
Action(328-374)src/app/endpoints/conversations.py (2)
get_conversation_endpoint_handler(214-343)delete_conversation_endpoint_handler(350-471)
tests/unit/models/responses/test_unauthorized_response.py (1)
src/models/responses.py (2)
UnauthorizedResponse(1081-1104)DetailModel(958-962)
⏰ 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). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (5)
tests/e2e/features/query.feature (1)
71-72: Correct 404 and message for missing conversationExpectation now matches NotFoundResponse payload. Good.
tests/e2e/features/conversations.feature (1)
117-117: LGTM: 400 for malformed IDs + auth header before DELETEThese updates align with the new error model and authorization flow.
Also applies to: 156-156, 160-160
tests/unit/models/responses/test_unauthorized_response.py (1)
3-3: LGTM: Tests align with DetailModel-based UnauthorizedResponseCovers both default and user-specific causes; assertions are precise.
Also applies to: 9-14, 16-21
docs/openapi.json (1)
696-704: LGTM: Standardized error models and updated responses for conversationsPaths now reference DetailModel-based errors (400/403/404/503) and the new schemas are defined under components. Looks coherent.
Also applies to: 749-757, 759-767, 779-787, 789-797, 799-807, 840-848, 850-858, 870-878, 880-888, 890-898, 1313-1333, 1620-1640, 2317-2336, 2985-3005, 3768-3787, 3975-3989
src/app/endpoints/conversations.py (1)
249-279: Past review comments are now resolved.The critical issue raised in previous reviews about 404 errors for authorized cross-user access has been fixed. The
retrieve_conversationfunction signature has been updated to only takeconversation_id(nouser_idparameter), and it now queries solely by ID. The current implementation correctly handles cross-user access:
can_access_conversationchecks permissions (returnsTruewhenREAD_OTHERS_CONVERSATIONSis granted)retrieve_conversationfetches by ID only (no user_id filter)- Returns 404 if conversation doesn't exist
This is the correct approach and resolves the previously flagged bug.
| "authorization.resolvers.NoopAccessResolver.get_actions", | ||
| return_value=set(Action.GET_CONVERSATION), | ||
| ) # Reduce user's permissions to access only their conversations |
There was a problem hiding this comment.
Forbidden/allowed tests won’t hit intended branches; mock the middleware resolvers instead
authorize middleware overwrites request.state.authorized_actions using get_authorization_resolvers(). Patching NoopAccessResolver.get_actions is ineffective here; 403 branches won’t trigger.
Patch the middleware factory to return resolvers with the desired action sets:
@@
- mocker.patch(
- "authorization.resolvers.NoopAccessResolver.get_actions",
- return_value=set(Action.GET_CONVERSATION),
- ) # Reduce user's permissions to access only their conversations
+ # Only allow reading own conversations (no READ_OTHERS_CONVERSATIONS)
+ mock_role_resolver = mocker.AsyncMock()
+ mock_role_resolver.resolve_roles.return_value = set()
+ mock_access_resolver = mocker.Mock()
+ mock_access_resolver.check_access.return_value = True
+ mock_access_resolver.get_actions.return_value = {Action.GET_CONVERSATION}
+ mocker.patch(
+ "authorization.middleware.get_authorization_resolvers",
+ return_value=(mock_role_resolver, mock_access_resolver),
+ )
@@
- mocker.patch(
- "authorization.resolvers.NoopAccessResolver.get_actions",
- return_value={Action.GET_CONVERSATION, Action.READ_OTHERS_CONVERSATIONS},
- ) # Allow user to access other users' conversations
+ # Allow reading others' conversations
+ mock_role_resolver = mocker.AsyncMock()
+ mock_role_resolver.resolve_roles.return_value = set()
+ mock_access_resolver = mocker.Mock()
+ mock_access_resolver.check_access.return_value = True
+ mock_access_resolver.get_actions.return_value = {
+ Action.GET_CONVERSATION, Action.READ_OTHERS_CONVERSATIONS
+ }
+ mocker.patch(
+ "authorization.middleware.get_authorization_resolvers",
+ return_value=(mock_role_resolver, mock_access_resolver),
+ )
@@
- mocker.patch(
- "authorization.resolvers.NoopAccessResolver.get_actions",
- return_value=set(Action.DELETE_CONVERSATION),
- ) # Reduce user's permissions to delete only their conversations
+ # Only allow deleting own conversations (no DELETE_OTHERS_CONVERSATIONS)
+ mock_role_resolver = mocker.AsyncMock()
+ mock_role_resolver.resolve_roles.return_value = set()
+ mock_access_resolver = mocker.Mock()
+ mock_access_resolver.check_access.return_value = True
+ mock_access_resolver.get_actions.return_value = {Action.DELETE_CONVERSATION}
+ mocker.patch(
+ "authorization.middleware.get_authorization_resolvers",
+ return_value=(mock_role_resolver, mock_access_resolver),
+ )
@@
- mocker.patch(
- "authorization.resolvers.NoopAccessResolver.get_actions",
- return_value={
- Action.DELETE_OTHERS_CONVERSATIONS,
- Action.DELETE_CONVERSATION,
- },
- ) # Allow user to detele other users' conversations
+ # Allow deleting others' conversations
+ mock_role_resolver = mocker.AsyncMock()
+ mock_role_resolver.resolve_roles.return_value = set()
+ mock_access_resolver = mocker.Mock()
+ mock_access_resolver.check_access.return_value = True
+ mock_access_resolver.get_actions.return_value = {
+ Action.DELETE_CONVERSATION, Action.DELETE_OTHERS_CONVERSATIONS
+ }
+ mocker.patch(
+ "authorization.middleware.get_authorization_resolvers",
+ return_value=(mock_role_resolver, mock_access_resolver),
+ )Also applies to: 454-456, 678-680, 716-721
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_conversations.py around lines 411-413 (and also
apply same change at 454-456, 678-680, 716-721), the test currently patches
NoopAccessResolver.get_actions which is ineffective because the authorize
middleware overwrites request.state.authorized_actions via
get_authorization_resolvers(); instead patch get_authorization_resolvers (the
middleware factory) to return a list of resolver instances or callables whose
get_actions returns the desired action set for each test case so the middleware
populates request.state.authorized_actions with the intended values and the
403/allowed branches are exercised.
2 participants
Description
Fix GET and DELETE conversation endpoints to return correct HTTP status codes.
Update conversations response models with structured detail field including response and cause.
Update endpoint docstrings, OpenAPI specification; unit, integration and e2e tests.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests