Skip to content

Conversation

@asimurka
Copy link

@asimurka asimurka commented Oct 24, 2025

Description

This commit updates the feedback response dictionaries, adds an end-to-end test verifying a 400 status code for the PUT endpoint, and fixes a bug in the existing e2e tests.

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

Related Tickets & Documents

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

    • Improved unauthorized feedback error to return structured, clearer error details.
    • Added a specific error response for failures to store feedback (invalid storage path).
  • Improvements

    • Aligned feedback API response declarations for POST/GET/PUT endpoints for consistency.
  • Tests

    • E2E tests: environment-driven host/port configuration added and assertions adjusted for feedback status updates.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 24, 2025

Walkthrough

The PR renames the POST response mapping and adds distinct GET and PUT response mappings for the feedback endpoints, introduces an InvalidFeedbackStoragePathResponse error model, and adjusts E2E test environment, scenarios, and step assertions related to feedback authorization and status updates.

Changes

Cohort / File(s) Summary
Feedback endpoint mappings
src/app/endpoints/feedback.py
Renamed feedback_responsefeedback_post_response; added feedback_get_response and feedback_put_response; updated @router.post("", ...), @router.get("/status", ...), and @router.put("/status", ...) to use new response mappings.
Error response models
src/models/responses.py
Added InvalidFeedbackStoragePathResponse(AbstractErrorResponse) with __init__(storage_path: str) producing a "Failed to store feedback" detail/cause and an example in model_config.json_schema_extra.
E2E test environment
tests/e2e/features/environment.py
Added context.hostname and context.port in before_feature from E2E_LSC_HOSTNAME/E2E_LSC_PORT with defaults; removed a debug print in after_feature.
E2E feature scenarios
tests/e2e/features/feedback.feature
Replaced plain-string unauthorized error detail with structured object (response, cause); added scenario checking unauthorized PUT /status returns structured error.
E2E step implementations
tests/e2e/features/steps/feedback.py
Removed explicit assertions for HTTP 200 after PUT requests that enable/disable feedback; other step logic unchanged.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Router
  participant FeedbackService
  participant ResponsesModel

  rect #f0f7ff
    Client->>Router: POST /feedback (auth)
    Router->>FeedbackService: handle_post_feedback()
    FeedbackService-->>ResponsesModel: use feedback_post_response (200/401/403/500)
    FeedbackService-->>Router: response
    Router-->>Client: 200 | 401 | 403 | 500
  end

  rect #f7fff0
    Client->>Router: GET /feedback/status
    Router->>FeedbackService: handle_get_status()
    FeedbackService-->>ResponsesModel: use feedback_get_response (200)
    FeedbackService-->>Router: response
    Router-->>Client: 200
  end

  rect #fff7f0
    Client->>Router: PUT /feedback/status (auth)
    Router->>FeedbackService: handle_put_status()
    alt invalid storage path
      FeedbackService-->>ResponsesModel: InvalidFeedbackStoragePathResponse(storage_path)
      FeedbackService-->>Router: 500 error with detail
      Router-->>Client: 500 (Failed to store feedback)
    else normal flow
      FeedbackService-->>ResponsesModel: use feedback_put_response (200/400/401/403)
      FeedbackService-->>Router: response
      Router-->>Client: 200 | 400 | 401 | 403
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • tisnik

Poem

🐰 I hopped through routes and mapped each post,
I stitched new errors when storage is lost,
Tests learned the host and port to call,
Status checks now answer one and all,
A tiny carrot for a tidy code trot 🥕

Pre-merge checks and finishing touches

✅ 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 "Updated response dicts for feedback endpoints" accurately reflects the primary changes to the codebase. The changeset includes renaming feedback_response to feedback_post_response, adding feedback_put_response and feedback_get_response, and updating route declarations to use these new response mappings in src/app/endpoints/feedback.py. The title is specific and clear about what is being updated and where, directly corresponding to the stated PR objective of updating feedback response dictionaries. While the changeset also includes a new error response class and test improvements, the title appropriately focuses on the core change affecting the feedback endpoints.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

@asimurka asimurka force-pushed the fix_feedbacks_400_401_swagger_doc branch from 1dd12e2 to 6ca5d6a Compare October 24, 2025 13:18
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

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/feedback.feature (1)

259-268: Remove incorrect 400 status code mapping for authentication errors in feedback_put_response.

The review comment correctly identifies the issue. The feedback_put_response in src/app/endpoints/feedback.py (line 57-60) incorrectly maps HTTP 400 to UnauthorizedResponse.

Evidence:

  • UnauthorizedResponse class documentation explicitly states "401 Unauthorized - Missing or invalid credentials" (src/models/responses.py:1095)
  • The UnauthorizedResponse error message "Missing or invalid credentials provided by client" is an authentication error, not a malformed request
  • The authentication implementations (src/authentication/k8s.py, src/authentication/jwk_token.py) correctly raise status code 401 for missing/invalid credentials
  • HTTP standards dictate 401 for missing/invalid authentication, 400 for malformed requests
  • The feedback_put_response already has a correct 401 mapping to UnauthorizedResponse (line 61-64)

Required fixes:

  • Remove the 400 status code mapping from feedback_put_response (lines 57-60 in src/app/endpoints/feedback.py), or replace it with a BadRequestResponse model if a 400 status is intentional
  • Update the test in tests/e2e/features/feedback.feature (lines 259-268) to expect HTTP 401 instead of 400 for missing authentication
♻️ Duplicate comments (1)
tests/e2e/features/feedback.feature (1)

270-283: Same authentication status code issue as Line 259.

This scenario has the same concern: expecting 400 for missing authentication instead of 401.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45c7482 and 6ca5d6a.

📒 Files selected for processing (5)
  • src/app/endpoints/feedback.py (5 hunks)
  • src/models/responses.py (1 hunks)
  • tests/e2e/features/environment.py (1 hunks)
  • tests/e2e/features/feedback.feature (1 hunks)
  • tests/e2e/features/steps/feedback.py (0 hunks)
💤 Files with no reviewable changes (1)
  • tests/e2e/features/steps/feedback.py
🧰 Additional context used
📓 Path-based instructions (9)
**/*.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/e2e/features/environment.py
  • src/app/endpoints/feedback.py
  • src/models/responses.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/e2e/features/environment.py
tests/e2e/features/**/*.feature

📄 CodeRabbit inference engine (CLAUDE.md)

Write E2E tests as Gherkin feature files for behave

Files:

  • tests/e2e/features/feedback.feature
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/feedback.py
  • 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/feedback.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/feedback.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/feedback.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
🧬 Code graph analysis (1)
src/app/endpoints/feedback.py (1)
src/models/responses.py (4)
  • FeedbackStatusUpdateResponse (897-933)
  • UnauthorizedResponse (1094-1117)
  • ForbiddenResponse (1120-1142)
  • StatusResponse (533-571)
⏰ 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 (5)
src/app/endpoints/feedback.py (4)

33-50: LGTM!

The rename from feedback_response to feedback_post_response improves clarity by making the endpoint association explicit.


112-112: LGTM!

The route correctly references the renamed feedback_post_response mapping.


190-190: LGTM!

The route correctly uses the new feedback_get_response mapping.


208-208: LGTM!

The route correctly uses the new feedback_put_response mapping.

tests/e2e/features/environment.py (1)

148-149: LGTM!

The environment-based configuration for hostname and port follows the existing pattern and provides appropriate defaults for E2E testing.

Comment on lines +52 to +69
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,
},
}
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).

@asimurka asimurka force-pushed the fix_feedbacks_400_401_swagger_doc branch from 6ca5d6a to 8eb46d4 Compare October 24, 2025 15:11
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

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/feedback.feature (1)

246-268: Reconsider using 400 for authentication failures.

The scenario expects a 400 (Bad Request) status code for missing authorization headers, but per HTTP semantics, authentication failures should return 401 (Unauthorized). Status code 400 is reserved for malformed request syntax or invalid request parameters, not missing/invalid credentials.

Consider updating the test to expect 401 instead:

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

This aligns with standard HTTP conventions and the UnauthorizedResponse model being used.

♻️ Duplicate comments (1)
src/app/endpoints/feedback.py (1)

52-69: Address the duplicate and semantically incorrect 400 status code entry.

Lines 57-60 define a 400 response with UnauthorizedResponse model and an authentication failure description. This creates two issues:

  1. Semantic incorrectness: HTTP 400 represents malformed requests or validation errors, not authentication failures. Missing/invalid credentials should return 401.
  2. Duplication: The 400 entry (lines 57-60) and 401 entry (lines 61-64) have identical descriptions and models, which is redundant and confusing.

Per the past review comment, either:

  • Remove the 400 entry if it's not needed for validation errors, OR
  • Update the 400 entry to use BadRequestResponse with a description about malformed request bodies
-    400: {
-        "description": "Missing or invalid credentials provided by client",
-        "model": UnauthorizedResponse,
-    },
         "description": "Missing or invalid credentials provided by client",
         "model": UnauthorizedResponse,
     },
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6ca5d6a and 8eb46d4.

📒 Files selected for processing (5)
  • src/app/endpoints/feedback.py (5 hunks)
  • src/models/responses.py (1 hunks)
  • tests/e2e/features/environment.py (1 hunks)
  • tests/e2e/features/feedback.feature (1 hunks)
  • tests/e2e/features/steps/feedback.py (0 hunks)
💤 Files with no reviewable changes (1)
  • tests/e2e/features/steps/feedback.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/e2e/features/environment.py
🧰 Additional context used
📓 Path-based instructions (8)
tests/e2e/features/**/*.feature

📄 CodeRabbit inference engine (CLAUDE.md)

Write E2E tests as Gherkin feature files for behave

Files:

  • tests/e2e/features/feedback.feature
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)

Files:

  • src/models/responses.py
  • src/app/endpoints/feedback.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/models/responses.py
  • src/app/endpoints/feedback.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/feedback.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/feedback.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/feedback.py
🧬 Code graph analysis (1)
src/app/endpoints/feedback.py (1)
src/models/responses.py (4)
  • FeedbackStatusUpdateResponse (897-933)
  • UnauthorizedResponse (1094-1117)
  • ForbiddenResponse (1120-1142)
  • StatusResponse (533-571)
⏰ 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 (4)
src/models/responses.py (1)

1145-1171: LGTM! The docstring has been corrected.

The implementation follows the established pattern for error response classes and provides clear, structured error information for feedback storage failures. The previous docstring issue has been addressed correctly.

src/app/endpoints/feedback.py (3)

33-50: LGTM! Clear naming improves maintainability.

Renaming to feedback_post_response clearly indicates this mapping is specific to the POST endpoint, improving code readability and maintainability.


71-76: LGTM! The typo has been corrected.

The response mapping correctly uses "retrieved" (the past review comment about the typo has been addressed).


112-112: LGTM! Endpoint decorators correctly reference the new response mappings.

The route decorators have been properly updated to use the method-specific response mappings (feedback_post_response, feedback_get_response, feedback_put_response), maintaining consistency with the refactored response definitions.

Also applies to: 190-190, 208-208

Comment on lines +270 to 283
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"
}
}
"""
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).

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

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

LGTM

@tisnik tisnik merged commit aa3d770 into lightspeed-core:main Oct 24, 2025
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants