Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions src/app/endpoints/feedback.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
feedback_status_lock = threading.Lock()

# Response for the feedback endpoint
feedback_response: dict[int | str, dict[str, Any]] = {
feedback_post_response: dict[int | str, dict[str, Any]] = {
200: {
"description": "Feedback received and stored",
"model": FeedbackResponse,
Expand All @@ -49,6 +49,32 @@
},
}

feedback_put_response: dict[int | str, dict[str, Any]] = {
200: {
"description": "Feedback status successfully updated",
"model": FeedbackStatusUpdateResponse,
},
400: {
"description": "Missing or invalid credentials provided by client",
"model": UnauthorizedResponse,
},
401: {
"description": "Missing or invalid credentials provided by client",
"model": UnauthorizedResponse,
},
403: {
"description": "Client does not have permission to access resource",
"model": ForbiddenResponse,
},
}
Comment on lines +52 to +69
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Correct the 400 response entry in feedback_put_response.

The 400 status code entry (lines 57-60) has two issues:

  1. The description "Missing or invalid credentials provided by client" is semantically incorrect for HTTP 400, which represents malformed request syntax or invalid request parameters, not authentication failures.
  2. Using UnauthorizedResponse model for a 400 status creates confusion, as this model is designed for 401 responses.

HTTP status codes should follow standard conventions:

  • 400 Bad Request: Invalid request body, malformed JSON, validation errors
  • 401 Unauthorized: Missing or invalid authentication credentials
  • 403 Forbidden: Authenticated but not authorized

If 400 is intended for request validation errors (e.g., invalid body schema), update the description and use BadRequestResponse:

-        "description": "Missing or invalid credentials provided by client",
-        "model": UnauthorizedResponse,
+        "description": "Invalid request body or parameters",
+        "model": BadRequestResponse,
     },

If the intent is to document missing credentials, remove the 400 entry as 401 already covers this case.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
feedback_put_response: dict[int | str, dict[str, Any]] = {
200: {
"description": "Feedback status successfully updated",
"model": FeedbackStatusUpdateResponse,
},
400: {
"description": "Missing or invalid credentials provided by client",
"model": UnauthorizedResponse,
},
401: {
"description": "Missing or invalid credentials provided by client",
"model": UnauthorizedResponse,
},
403: {
"description": "Client does not have permission to access resource",
"model": ForbiddenResponse,
},
}
feedback_put_response: dict[int | str, dict[str, Any]] = {
200: {
"description": "Feedback status successfully updated",
"model": FeedbackStatusUpdateResponse,
},
400: {
"description": "Invalid request body or parameters",
"model": BadRequestResponse,
},
401: {
"description": "Missing or invalid credentials provided by client",
"model": UnauthorizedResponse,
},
403: {
"description": "Client does not have permission to access resource",
"model": ForbiddenResponse,
},
}
🤖 Prompt for AI Agents
In src/app/endpoints/feedback.py around lines 52 to 69, the 400 entry is
incorrect: replace the current description and model for the 400 status with a
BadRequest semantics (e.g., description like "Invalid request body or parameters
— malformed JSON or validation errors" and use the BadRequestResponse model) OR
if 400 was mistakenly added for authentication errors, remove the 400 entry
entirely since 401 already covers missing/invalid credentials; ensure remaining
status codes and models match standard HTTP semantics.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if other endpoints in the codebase return 400 for authentication issues
# Look for similar patterns in response dictionaries

rg -nP --type=py -C3 '400.*(?:description|model).*[Uu]nauthorized' 

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Look at the feedback feature test to understand the exact test scenario
cat tests/e2e/features/feedback.feature

Length of output: 9856


🏁 Script executed:

#!/bin/bash
# Check if there are other response dictionaries that use 400 with UnauthorizedResponse
ast-grep --pattern $'$_response: dict[$_, dict[$_, Any]] = {
  $$$
  400: {
    $$$
    "model": UnauthorizedResponse,
    $$$
  }
  $$$
}'

Length of output: 0


@asimurka, understood! I can see from the test scenarios (lines 301-330 and 332-350 in feedback.feature) that returning 400 for missing/invalid credentials is explicitly validated and intentional for the feedback endpoints. Thank you for clarifying this design decision.


🧬 Code Graph Analysis Results
  • File: src/utils/suid.py

    • Lines 6-16
    • Summary:
      • Function: get_suid() -> str
      • Description: Generates a UUID4 string suitable for use as a session identifier.
      • Returns: str UUID4
      • Notes: Standard RFC 4122 UUID format using uuid.uuid4().
  • File: src/models/requests.py

    • Lines 244-387

    • Summary:

      • Class: FeedbackRequest(BaseModel)
      • Description: Model for a feedback submission, including:
        • conversation_id: str (UUID format, validated)
        • user_question: str
        • llm_response: str
        • user_feedback: Optional[str]
        • sentiment: Optional[int] (-1, 1, or None)
        • categories: Optional[List[FeedbackCategory]] (non-empty list normalized, supports negative feedback categories)
      • Validation:
        • conversation_id must pass RFC 4122/SUID format checks
        • sentiment must be -1, 1, or None
        • categories validated for duplicates/ordering
        • After-model validation requires at least one of sentiment, user_feedback, or categories
      • Notes: Includes model_config with examples, extra forbidding, and field validators.
    • File: src/models/requests.py

    • Lines 390-415

    • Summary:

      • Class: FeedbackStatusUpdateRequest(BaseModel)
      • Description: Model for updating feedback enabled status
        • status: bool (desired enabled state)
      • Validation:
        • extra: forbid
      • Methods:
        • get_value() -> bool: returns the status value
  • File: src/models/config.py

    • Lines 329-375
    • Summary:
      • Enum: Action (string-based)
      • Description: Defines available actions in the system, including:
        • ADMIN, FEEDBACK, and various read/write/query/update actions
      • Notes: FEEDBACK is the specific action used by the feedback endpoint; ADMIN is a superuser override.
  • File: src/authorization/middleware.py

    • Lines 111-122
    • Summary:
      • Function: authorize(action: Action) -> Callable
      • Description: Decorator factory that returns a decorator which wraps a route function with an authorization check.
      • Behavior: On call, performs authorization check via _perform_authorization_check(action, args, kwargs) and only proceeds if authorized.
  • File: src/authentication/init.py

    • Lines 14-52
    • Summary:
      • Function: get_auth_dependency(virtual_path: str = constants.DEFAULT_VIRTUAL_PATH) -> AuthInterface
      • Description: Returns the configured authentication dependency instance based on the loaded configuration.
      • Behavior:
        • Loads configuration if not already loaded (including env-based path fallback)
        • Instantiates one of several authentication dependency classes (noop, noop_with_token, k8s, jwk_token) based on module
      • Returns: An object implementing the authentication interface for dependency injection.
  • File: src/configuration.py

    • Lines 65-69
    • Summary:
      • Property: configuration(self) -> Configuration
      • Description: Returns the loaded configuration; raises LogicError if not loaded
    • Note: Used to access top-level configuration object.
  • File: src/configuration.py

    • Lines 86-90
    • Summary:
      • Property: user_data_collection_configuration(self) -> UserDataCollection
      • Description: Returns the user data collection configuration; raises LogicError if not loaded
    • Note: Used to access feedback/storage-related configuration (e.g., feedback_enabled flag, storage path).


feedback_get_response: dict[int | str, dict[str, Any]] = {
200: {
"description": "Feedback status successfully retrieved",
"model": StatusResponse,
}
}


def is_feedback_enabled() -> bool:
"""
Expand Down Expand Up @@ -83,7 +109,7 @@ async def assert_feedback_enabled(_request: Request) -> None:
)


@router.post("", responses=feedback_response)
@router.post("", responses=feedback_post_response)
@authorize(Action.FEEDBACK)
async def feedback_endpoint_handler(
feedback_request: FeedbackRequest,
Expand Down Expand Up @@ -161,7 +187,7 @@ def store_feedback(user_id: str, feedback: dict) -> None:
logger.info("Feedback stored successfully at %s", feedback_file_path)


@router.get("/status")
@router.get("/status", responses=feedback_get_response)
def feedback_status() -> StatusResponse:
"""
Handle feedback status requests.
Expand All @@ -179,7 +205,7 @@ def feedback_status() -> StatusResponse:
)


@router.put("/status")
@router.put("/status", responses=feedback_put_response)
@authorize(Action.ADMIN)
async def update_feedback_status(
feedback_update_request: FeedbackStatusUpdateRequest,
Expand Down
29 changes: 29 additions & 0 deletions src/models/responses.py
Original file line number Diff line number Diff line change
Expand Up @@ -1140,3 +1140,32 @@ def __init__(self, user_id: str, resource: str, resource_id: str):
]
}
}


class InvalidFeedbackStoragePathResponse(AbstractErrorResponse):
"""500 Internal Error - Invalid feedback storage path."""

def __init__(self, storage_path: str):
"""Initialize an InvalidFeedbackStoragePathResponse for feedback storage failures."""
super().__init__(
detail=DetailModel(
response="Failed to store feedback",
cause=f"Invalid feedback storage path: {storage_path}",
)
)

model_config = {
"json_schema_extra": {
"examples": [
{
"detail": {
"response": "Failed to store feedback",
"cause": (
"Invalid feedback storage path: "
"/var/app/data/feedbacks/invalid_path"
),
}
}
]
}
}
3 changes: 2 additions & 1 deletion tests/e2e/features/environment.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ def before_feature(context: Context, feature: Feature) -> None:
restart_container("lightspeed-stack")

if "Feedback" in feature.tags:
context.hostname = os.getenv("E2E_LSC_HOSTNAME", "localhost")
context.port = os.getenv("E2E_LSC_PORT", "8080")
context.feedback_conversations = []


Expand All @@ -156,7 +158,6 @@ def after_feature(context: Context, feature: Feature) -> None:
remove_config_backup(context.default_config_backup)

if "Feedback" in feature.tags:
print(context.feedback_conversations)
for conversation_id in context.feedback_conversations:
url = f"http://{context.hostname}:{context.port}/v1/conversations/{conversation_id}"
headers = context.auth_headers if hasattr(context, "auth_headers") else {}
Expand Down
20 changes: 19 additions & 1 deletion tests/e2e/features/feedback.feature
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,25 @@ Feature: feedback endpoint API tests
And The body of the response is the following
"""
{
"detail": "No Authorization header found"
"detail": {
"cause": "Missing or invalid credentials provided by client",
"response": "Unauthorized"
}
}
"""

Scenario: Check if update feedback status endpoint is not working when not authorized
Given The system is in default state
And I remove the auth header
When The feedback is enabled
Then The status code of the response is 400
And The body of the response is the following
"""
{
"detail": {
"cause": "Missing or invalid credentials provided by client",
"response": "Unauthorized"
}
}
"""
Comment on lines +270 to 283
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reconsider using 400 for authentication failures.

This new scenario also expects a 400 status code for missing credentials, which is semantically incorrect per HTTP standards. Authentication failures should return 401 (Unauthorized), not 400 (Bad Request).

Apply this change:

-     Then The status code of the response is 400
+     Then The status code of the response is 401
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Scenario: Check if update feedback status endpoint is not working when not authorized
Given The system is in default state
And I remove the auth header
When The feedback is enabled
Then The status code of the response is 400
And The body of the response is the following
"""
{
"detail": {
"cause": "Missing or invalid credentials provided by client",
"response": "Unauthorized"
}
}
"""
Scenario: Check if update feedback status endpoint is not working when not authorized
Given The system is in default state
And I remove the auth header
When The feedback is enabled
Then The status code of the response is 401
And The body of the response is the following
"""
{
"detail": {
"cause": "Missing or invalid credentials provided by client",
"response": "Unauthorized"
}
}
"""
🤖 Prompt for AI Agents
In tests/e2e/features/feedback.feature around lines 270 to 283, the scenario
asserts a 400 status for missing/invalid credentials which is incorrect; change
the expected HTTP status from 400 to 401 and update any scenario title or
assertions to reflect an authentication failure (e.g., "not authorized" ->
"unauthorized") while keeping the response body assertion consistent (ensure the
response.detail.response remains "Unauthorized" and cause text is appropriate).


Expand Down
2 changes: 0 additions & 2 deletions tests/e2e/features/steps/feedback.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ def enable_feedback(context: Context) -> None:
assert context is not None
payload = {"status": True}
access_feedback_put_endpoint(context, payload)
assert context.response.status_code == 200, "Enabling feedback was unsuccessful"


@step("The feedback is disabled") # type: ignore
Expand All @@ -26,7 +25,6 @@ def disable_feedback(context: Context) -> None:
assert context is not None
payload = {"status": False}
access_feedback_put_endpoint(context, payload)
assert context.response.status_code == 200, "Disabling feedback was unsuccessful"


@when("I update feedback status with") # type: ignore
Expand Down
Loading