[LCORE 756] Before storing feedback, validate that convo exists & belongs to user #1005
Conversation
WalkthroughAdds conversation existence and ownership validation to the feedback submission endpoint: the endpoint now retrieves the conversation by ID, returns 404 if missing, returns 403 if owned by a different user, and only stores feedback after these checks. Tests and E2E steps were updated to exercise these cases. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant FeedbackAPI as Feedback Endpoint
participant Retriever as Conversation Retriever
participant Auth as Auth/User Context
participant Store as Feedback Store
Client->>FeedbackAPI: POST /feedback (conversation_id, feedback)
FeedbackAPI->>Auth: determine request user_id
FeedbackAPI->>Retriever: retrieve_conversation(conversation_id)
alt conversation not found
Retriever-->>FeedbackAPI: not found
FeedbackAPI-->>Client: 404 Conversation not found
else conversation found
Retriever-->>FeedbackAPI: conversation (with owner_id)
FeedbackAPI->>Auth: compare owner_id vs requester user_id
alt owner mismatch
FeedbackAPI-->>Client: 403 Forbidden (permission denied)
else owner matches
FeedbackAPI->>Store: persist feedback
Store-->>FeedbackAPI: success
FeedbackAPI-->>Client: 200/201 OK
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/unit/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-01-11T16:30:41.784ZApplied to files:
⏰ 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). (5)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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 |
Apply CodeRabbit suggestion to combine two DB queries into one by checking conversation.user_id directly instead of calling validate_conversation_ownership. Update unit tests to mock retrieve_conversation to avoid database initialization errors during testing. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/app/endpoints/test_feedback.py (1)
105-143: Missing test coverage for conversation validation error cases.The PR adds validation that returns 404 when conversation doesn't exist and 403 when conversation belongs to a different user, but no tests cover these error paths. Per coding guidelines, unit tests should cover new functionality.
Consider adding tests for the new validation logic:
`@pytest.mark.asyncio` async def test_feedback_endpoint_handler_conversation_not_found( mocker: MockerFixture, ) -> None: """Test that feedback_endpoint_handler returns 404 when conversation doesn't exist.""" mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.feedback.assert_feedback_enabled", return_value=None) mocker.patch("app.endpoints.feedback.retrieve_conversation", return_value=None) feedback_request = mocker.Mock() feedback_request.conversation_id = "12345678-abcd-0000-0123-456789abcdef" auth: AuthTuple = ("test_user_id", "test_user", True, "test_token") with pytest.raises(HTTPException) as exc_info: await feedback_endpoint_handler( feedback_request=feedback_request, _ensure_feedback_enabled=assert_feedback_enabled, auth=auth, ) assert exc_info.value.status_code == status.HTTP_404_NOT_FOUND `@pytest.mark.asyncio` async def test_feedback_endpoint_handler_conversation_wrong_owner( mocker: MockerFixture, ) -> None: """Test that feedback_endpoint_handler returns 403 when conversation belongs to different user.""" mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.feedback.assert_feedback_enabled", return_value=None) mock_conversation = mocker.Mock() mock_conversation.user_id = "different_user_id" mocker.patch( "app.endpoints.feedback.retrieve_conversation", return_value=mock_conversation ) feedback_request = mocker.Mock() feedback_request.conversation_id = "12345678-abcd-0000-0123-456789abcdef" auth: AuthTuple = ("test_user_id", "test_user", True, "test_token") with pytest.raises(HTTPException) as exc_info: await feedback_endpoint_handler( feedback_request=feedback_request, _ensure_feedback_enabled=assert_feedback_enabled, auth=auth, ) assert exc_info.value.status_code == status.HTTP_403_FORBIDDEN#!/bin/bash # Verify the production code has 404 and 403 error handling that needs test coverage rg -n "HTTP_404_NOT_FOUND|HTTP_403_FORBIDDEN" src/app/endpoints/feedback.py
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/endpoints/feedback.pytests/unit/app/endpoints/test_feedback.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/endpoints/feedback.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
tests/unit/app/endpoints/test_feedback.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Usepytest-mockfor AsyncMock objects in tests
UseMOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")pattern for authentication mocks in tests
Files:
tests/unit/app/endpoints/test_feedback.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/**/*.py: Unit tests require 60% code coverage
Write unit tests covering new functionality before completion
Files:
tests/unit/app/endpoints/test_feedback.py
🧠 Learnings (1)
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/**/*.py : Use `MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")` pattern for authentication mocks in tests
Applied to files:
tests/unit/app/endpoints/test_feedback.py
⏰ 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). (5)
- GitHub Check: list_outdated_dependencies
- GitHub Check: unit_tests (3.13)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (3)
tests/unit/app/endpoints/test_feedback.py (3)
105-115: LGTM!The mock setup correctly aligns the conversation's
user_idwith the auth tuple's user ID ("test_user_id"), ensuring the ownership check passes. Theconversation_idassignment on line 115 properly links the request to the mocked conversation.
137-143: LGTM!The mock correctly sets up conversation ownership to pass validation, allowing the test to focus on verifying the
OSErrorhandling behavior.
319-325: LGTM!The mock correctly uses
"mock_user_id"which matchesMOCK_AUTH[0], ensuring consistency with the auth tuple used at line 330. This follows the recommendedMOCK_AUTHpattern from the coding guidelines.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
|
|
||
| @given("A conversation owned by a different user is initialized") # type: ignore | ||
| def initialize_conversation_different_user(context: Context) -> None: |
There was a problem hiding this comment.
move this into test steps within the feature file, this test step should not be needed at all
| user_id, _, _, _ = auth | ||
| check_configuration_loaded(configuration) | ||
|
|
||
| # Validate conversation exists and belongs to the user |
There was a problem hiding this comment.
this new logic should also be unit tested
3 participants
Description
Before storing feedback, validate that convo exists & belongs to user
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
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.