[LCORE 756] Before storing feedback, validate that convo exists & belongs to user

[max-svistunov]

Description

Before storing feedback, validate that convo exists & belongs to user

Type of change

• Refactor
• New feature
• Bug fix
• CVE fix
• Optimization
• Documentation Update
• Configuration Update
• Bump-up service version
• Bump-up dependent library
• Bump-up library or tool used for development (does not change the final image)
• CI configuration change
• Konflux configuration change
• Unit tests improvement
• Integration tests improvement
• End to end tests improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

• Assisted-by: Claude Opus 4.5
• Generated by: Claude Opus 4.5

Related Tickets & Documents

• Related Issue https://issues.redhat.com/browse/LCORE-756
• Closes https://issues.redhat.com/browse/LCORE-756

Checklist before requesting a review

• I have performed a self-review of my code.
• PR has passed all pre-merge test jobs.
• If it is a core feature, I have added thorough tests.

Testing

• Please provide detailed steps to perform tests related to this code change.
• How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

• Bug Fixes

  • Feedback submission now returns correct HTTP statuses and clearer error details: 404 when a conversation is missing, 403 when it belongs to another user, and 401 when unauthenticated.

• Tests

  • Expanded end-to-end and unit tests covering missing conversations, ownership/permission checks, and unauthenticated requests to ensure consistent behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The PR adds runtime validation to the feedback endpoint: it verifies the referenced conversation exists (returns 404) and that it belongs to the authenticated user (returns 403) before continuing to store feedback.

Changes

Cohort / File(s)	Summary
Feedback Endpoint Validation src/app/endpoints/feedback.py	Imports retrieve_conversation and adds pre-storage checks: return 404 if conversation missing, return 403 if conversation owner != authenticated user.
E2E Test Scenarios tests/e2e/features/feedback.feature	Updates nonexisting-conversation scenario to expect 404 and a detailed detail body; adds scenario for wrong-owner (expects 403); adjusts unauthorized scenarios to expect 401 and remove Authorization header.
E2E Step Helpers tests/e2e/features/steps/feedback.py	Adds initialize_conversation_with_user_id and create_conversation_with_user_id to allow creating conversations with an explicit user_id query parameter.
Unit Test Coverage tests/unit/app/endpoints/test_feedback.py	Mocks retrieve_conversation in multiple tests, sets feedback_request.conversation_id in tests, and adds tests for conversation-not-found (404) and wrong-owner (403).

Sequence Diagram(s)

mermaid
sequenceDiagram
actor Client
participant FeedbackEndpoint
participant Auth
participant ConversationService
participant Storage

Client->>Auth: send request with Authorization, conversation_id, feedback
Auth-->>FeedbackEndpoint: authenticated user info
FeedbackEndpoint->>ConversationService: retrieve_conversation(conversation_id)
ConversationService-->>FeedbackEndpoint: conversation (or not found)
alt conversation not found
    FeedbackEndpoint-->>Client: 404 Conversation not found
else conversation found but owner != authenticated user
    FeedbackEndpoint-->>Client: 403 Forbidden (ownership mismatch)
else conversation found and owner matches
    FeedbackEndpoint->>Storage: store feedback
    Storage-->>FeedbackEndpoint: stored confirmation
    FeedbackEndpoint-->>Client: 200/201 feedback stored
end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• LCORE-793: Refactored status codes and response models for v1/conversations #683: Introduces/updates retrieve_conversation used here to validate conversation existence and ownership.
• LCORE-354 Implement the /conversations endpoint #244: Implements conversation retrieval endpoint and conversation-to-agent mapping that the feedback endpoint's validation relies on.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and specifically describes the main change: adding validation that conversations exist and belong to the user before storing feedback.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

tests/e2e/features/steps/feedback.py (1)

113-122: Use query params to safely encode user_id.

Manual string concatenation won’t URL-encode special characters in user_id. Using params keeps this robust and clearer.

♻️ Proposed refactor

-    url = base + path
-    if user_id is not None:
-        url = f"{url}?user_id={user_id}"
+    url = base + path
+    params = {"user_id": user_id} if user_id is not None else None
...
-    response = requests.post(url, headers=headers, json=payload)
+    response = requests.post(url, headers=headers, json=payload, params=params)

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1b0b50 and af1ca2a.

📒 Files selected for processing (2)

• tests/e2e/features/feedback.feature
• tests/e2e/features/steps/feedback.py

🧰 Additional context used

📓 Path-based instructions (4)

**/*.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
Check constants.py for shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Use logger = 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, using typing_extensions.Self for model validators
Use union types with modern syntax: str | int instead of Union[str, int]
Use Optional[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
Use async def for I/O operations and external API calls
Handle APIConnectionError from Llama Stack in error handling
Use logger.debug() for detailed diagnostic information
Use logger.info() for general information about program execution
Use logger.warning() for unexpected events or potential problems
Use logger.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 @abstractmethod decorators
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
Run uv run make format to auto-format code with black and ruff before completion
Run uv run make verify to run all linters (black, pyl...

Files:

• tests/e2e/features/steps/feedback.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Use pytest-mock for AsyncMock objects in tests
Use MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") pattern for authentication mocks in tests

Files:

• tests/e2e/features/steps/feedback.py

tests/e2e/features/steps/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Define E2E test steps in tests/e2e/features/steps/ directory

Files:

• tests/e2e/features/steps/feedback.py

tests/e2e/features/**/*.feature

📄 CodeRabbit inference engine (CLAUDE.md)

Use behave (BDD) framework with Gherkin feature files for end-to-end tests

Files:

• tests/e2e/features/feedback.feature

🧠 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/e2e/features/steps/**/*.py : Define E2E test steps in `tests/e2e/features/steps/` directory

Applied to files:

• tests/e2e/features/steps/feedback.py

🧬 Code graph analysis (1)

tests/e2e/features/steps/feedback.py (1)

src/models/responses.py (1)

• endpoint (1470-1483)

⏰ 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: library mode / ci
• GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request

🔇 Additional comments (3)

tests/e2e/features/steps/feedback.py (1)

101-110: Clean delegation to a shared helper.

This keeps step definitions consistent and reduces duplication for user-specific setup.

tests/e2e/features/feedback.feature (2)

227-248: 404 expectation aligns with the new validation behavior.

Nice update to assert a not-found response and a specific cause.

---

250-267: This test expectation is correct—no change needed.

The feedback submission endpoint uses ForbiddenResponse.conversation() when a user attempts to submit feedback for a conversation they don't own, which returns the message "User does not have permission to perform this action". This matches exactly what the test expects on line 264.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[tisnik]

LGTM

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 719977e and 552bb04.

📒 Files selected for processing (2)

• src/app/endpoints/feedback.py
• tests/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
Check constants.py for shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Use logger = 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, using typing_extensions.Self for model validators
Use union types with modern syntax: str | int instead of Union[str, int]
Use Optional[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
Use async def for I/O operations and external API calls
Handle APIConnectionError from Llama Stack in error handling
Use logger.debug() for detailed diagnostic information
Use logger.info() for general information about program execution
Use logger.warning() for unexpected events or potential problems
Use logger.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 @abstractmethod decorators
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
Run uv run make format to auto-format code with black and ruff before completion
Run uv run make verify to 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
Use pytest-mock for AsyncMock objects in tests
Use MOCK_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_id with the auth tuple's user ID ("test_user_id"), ensuring the ownership check passes. The conversation_id assignment 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 OSError handling behavior.

---

319-325: LGTM!

The mock correctly uses "mock_user_id" which matches MOCK_AUTH[0], ensuring consistency with the auth tuple used at line 330. This follows the recommended MOCK_AUTH pattern from the coding guidelines.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[radofuchs]

LGTM