Use correct status codes for /query endpoint #776
Conversation
WalkthroughAuthentication failures now return HTTP 401 Unauthorized instead of 400 Bad Request; missing or unavailable model/provider scenarios return HTTP 404 Not Found instead of 400. Added explicit handling and E2E config for a "no models" runtime; tests and OpenAPI updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as App/Endpoint
participant Auth as Auth Utils / JWK
rect rgb(235,247,255)
Note right of Auth `#DFF7FF`: Authentication flow (401 on failures)
Client->>API: Request (missing/invalid Authorization)
API->>Auth: extract / decode token
Auth-->>API: raises DecodeError / missing header
API-->>Client: HTTP 401 Unauthorized (error detail)
end
sequenceDiagram
autonumber
participant Client
participant API as App/Endpoint
participant Registry as Model Registry
rect rgb(255,250,240)
Note right of API `#FFF4E6`: Model selection flow (404 for missing resources)
Client->>API: Query (optional model/provider)
API->>Registry: list available models
alt no models configured
Registry-->>API: []
API-->>Client: HTTP 404 Not Found ("No models available")
else models exist
API->>Registry: match requested model/provider or derive from config
alt model/provider not found
API-->>Client: HTTP 404 Not Found ("model/provider not found in available models")
else found
API-->>Client: Proceed with selected model (200)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
src/app/endpoints/query.py (1)
69-95: Document the new 401/404 responses
query_endpoint_handlernow emits 401 for auth failures and 404 when no suitable model exists, but the OpenAPI response map still advertises a 400. That leaves the API spec out of sync with runtime behavior and will confuse generated clients. Please updatequery_responseto describe the 401 Unauthorized (reuseUnauthorizedResponse) and add a 404 entry for model/provider misses, removing the obsolete 400 mapping.Apply this diff to keep the schema aligned:
query_response: dict[int | str, dict[str, Any]] = { "conversation_id": "123e4567-e89b-12d3-a456-426614174000", "response": "LLM answer", "referenced_documents": [ { "doc_url": "https://docs.openshift.com/" "container-platform/4.15/operators/olm/index.html", "doc_title": "Operator Lifecycle Manager (OLM)", } ], }, - 400: { - "description": "Missing or invalid credentials provided by client", - "model": UnauthorizedResponse, - }, + 401: { + "description": "Missing or invalid credentials provided by client", + "model": UnauthorizedResponse, + }, "description": "User is not authorized", "model": ForbiddenResponse, }, + 404: { + "description": "Requested model or provider not found", + }, "detail": { "response": "Unable to connect to Llama Stack", "cause": "Connection error.", }tests/e2e/features/query.feature (1)
96-96: Fix inconsistent scenario indentation.The scenario starting at line 96 is not properly indented, breaking consistency with other scenarios in the file.
Apply this diff:
-Scenario: Check if LLM responds for query request with error for missing query + Scenario: Check if LLM responds for query request with error for missing query
🧹 Nitpick comments (1)
tests/e2e/features/query.feature (1)
165-175: Consider refactoring scenario with special configuration needs.The scenario re-declares Background steps (lines 166-168), which is unusual in Gherkin as Background automatically applies to all scenarios. This suggests the scenario requires special configuration (e.g.,
no-models-run.yaml), but the current approach is confusing.Consider these alternatives for clearer test organization:
- Use tags to indicate special configuration:
@no_models Scenario: Check if LLM responds with an error when no models are configured Given The system is in default state And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva When I use "query" to ask question with authorization header """ {"query": "Write a simple code for reversing string"} """ Then The status code of the response is 404 And The body of the response contains No models available
- Move to a separate feature file (e.g.,
query_no_models.feature) if this configuration requires substantially different setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/app/endpoints/query.py(3 hunks)src/authentication/jwk_token.py(1 hunks)src/authentication/utils.py(1 hunks)tests/e2e/features/environment.py(1 hunks)tests/e2e/features/query.feature(2 hunks)tests/unit/app/endpoints/test_authorized.py(1 hunks)tests/unit/authentication/test_jwk_token.py(1 hunks)tests/unit/authentication/test_noop_with_token.py(2 hunks)tests/unit/authentication/test_utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-25T09:11:38.701Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 445
File: tests/e2e/features/environment.py:0-0
Timestamp: 2025-08-25T09:11:38.701Z
Learning: In the Behave Python testing framework, the Context object is created once for the entire test run and reused across all features and scenarios. Custom attributes added to the context persist until explicitly cleared, so per-scenario state should be reset in hooks to maintain test isolation.
Applied to files:
tests/e2e/features/environment.py
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/unit/app/endpoints/test_authorized.pytests/unit/authentication/test_noop_with_token.py
📚 Learning: 2025-09-02T11:15:02.411Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.411Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
Applied to files:
tests/unit/authentication/test_noop_with_token.py
🧬 Code graph analysis (1)
tests/unit/app/endpoints/test_authorized.py (1)
src/authentication/utils.py (1)
extract_user_token(7-26)
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (2)
tests/e2e/features/query.feature (2)
56-56: LGTM: Correct status code for missing authentication.The change from 400 to 401 correctly reflects that missing authentication should return
401 Unauthorizedper HTTP standards.
75-84: LGTM: Correct status code for non-existent model/provider.The scenario correctly uses
404 Not Foundfor missing resources, aligning with HTTP standards and the PR objective.
max-svistunov
force-pushed
the
lcore-702-query-errors
branch
from
204a2a2 to
075c629
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/endpoints/query.py(4 hunks)tests/e2e/features/query.feature(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/features/query.feature
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.411Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
🧬 Code graph analysis (1)
src/app/endpoints/query.py (1)
src/models/responses.py (2)
UnauthorizedResponse(1094-1117)ForbiddenResponse(1120-1142)
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (2)
src/app/endpoints/query.py (2)
81-91: LGTM! Response schema correctly documents new status codes.The additions of 401 (Unauthorized) and 404 (Not Found) to the response schema align with more semantically correct HTTP status code usage for authentication failures and missing resources.
501-543: LGTM! Status codes now semantically correct for missing resources.The changes from 400 to 404 for missing model/provider scenarios are appropriate:
- Line 521: Returns 404 when no LLM-type model exists in the available models list
- Line 538: Returns 404 when the specified model/provider combination is not found
Using 404 (Not Found) instead of 400 (Bad Request) correctly signals that the requested resource (model/provider) doesn't exist, rather than indicating a malformed request.
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 (2)
docs/openapi.json (2)
4073-4093: UnauthorizedResponse schema doesn’t match actual responses; allow string or object.E2E tests assert bodies like {"detail": "No Authorization header found"}, but the schema requires an object with response/cause. Update the schema to accept both.
"UnauthorizedResponse": { "properties": { - "detail": { "$ref": "#/components/schemas/DetailModel" } + "detail": { + "anyOf": [ + { "$ref": "#/components/schemas/DetailModel" }, + { "type": "string" } + ], + "title": "Detail" + } }, "type": "object", "required": ["detail"], "title": "UnauthorizedResponse", "description": "401 Unauthorized - Missing or invalid credentials.", ... }
1920-1923: Type mismatch: last_message_timestamp is number but examples show ISO 8601 strings.ConversationsListResponseV2 examples use a timestamp string, while ConversationData.last_message_timestamp is typed as number. Use string with date-time format for clarity and interoperability.
"ConversationData": { "properties": { ... - "last_message_timestamp": { "type": "number", "title": "Last Message Timestamp" } + "last_message_timestamp": { + "type": "string", + "format": "date-time", + "title": "Last Message Timestamp" + } }, ... }Also applies to: 955-969
🧹 Nitpick comments (5)
docs/openapi.json (1)
378-380: Add response schema to 404 for /v1/query.Return a structured error body like other 404s (e.g., NotFoundResponse) to keep the API consistent.
"404": { - "description": "Requested model or provider not found" + "description": "Requested model or provider not found", + "content": { + "application/json": { + "schema": { "$ref": "#/components/schemas/NotFoundResponse" } + } + } }tests/integration/test_openapi_json.py (1)
123-170: Add coverage for /v2/query and new feedback/status PUT codes.
- Include /v2/query POST in both parameterized lists so spec drift is caught.
- Consider expanding feedback/status PUT expected set to include 400, 401, 403 now documented.
@@ ("/v1/feedback/status", "put", {"200", "422"}), + ("/v2/query", "post", {"200", "401", "403", "404", "422", "500"}), @@ ("/v1/feedback/status", "put", {"200", "422"}), + ("/v2/query", "post", {"200", "401", "403", "404", "422", "500"}),If you keep PUT minimal for now, at least add /v2/query so the new path doesn’t regress.
Also applies to: 178-226
tests/unit/app/endpoints/test_query.py (2)
136-141: Redundant configuration patch; simplify.You patch app.endpoints.query.configuration twice, then set to None. The first patch is overwritten. Remove it for clarity.
- mocker.patch( - "app.endpoints.query.configuration", - return_value=mocker.Mock(), - ) - mocker.patch("app.endpoints.query.configuration", None) + mocker.patch("app.endpoints.query.configuration", None)
1833-1853: Tighten test type hints (optional).Parameters annotated as list in test_evaluate_model_hints are tuples/objects. Use precise types to aid IDEs and future refactors.
-def test_evaluate_model_hints( - user_conversation: list, - request_values: list, - expected_values: list, -) -> None: +from typing import Optional, Tuple +def test_evaluate_model_hints( + user_conversation: Optional[UserConversation], + request_values: Tuple[Optional[str], Optional[str]], + expected_values: Tuple[Optional[str], Optional[str]], +) -> None:tests/e2e/features/query.feature (1)
134-144: Rename scenario: title doesn’t match steps (tests backend outage → 500).Current title says “missing provider” but the body disrupts llama-stack and asserts 500. Rename for clarity.
- Scenario: Check if LLM responds for query request with error for missing provider + Scenario: Check if LLM responds with an error when Llama Stack backend is unavailableNote: @Authorized tag use here is correct per e2e env behavior. Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/openapi.json(14 hunks)src/app/endpoints/query.py(4 hunks)tests/e2e/features/query.feature(2 hunks)tests/e2e/features/streaming_query.feature(1 hunks)tests/integration/test_openapi_json.py(2 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/endpoints/query.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
⏰ 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: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (5)
docs/openapi.json (1)
358-367: Good: /v1/query now uses 401 for auth failures and 404 for missing model/provider.These align with HTTP semantics and the PR goal. No further issues here.
Also applies to: 378-380
tests/integration/test_openapi_json.py (1)
132-133: Good: integration tests now expect 401 and 404 on /v1/query.These checks will catch regressions in the spec.
Also applies to: 189-189
tests/unit/app/endpoints/test_query.py (1)
461-462: Good: assertion aligns with new 404/no-models behavior.Matches the updated endpoint semantics.
tests/e2e/features/streaming_query.feature (1)
129-133: Good: unauthenticated streaming_query now returns 401.This matches the adjusted auth semantics.
tests/e2e/features/query.feature (1)
56-60: Good: scenarios reflect 401/404 semantics and no-models case.These E2E updates validate the intended behavior across common error paths.
Also applies to: 62-76, 77-88, 90-100, 171-180
| "/v2/query": { | ||
| "post": { | ||
| "tags": [ | ||
| "query_v2" | ||
| ], | ||
| "summary": "Query Endpoint Handler V2", | ||
| "description": "Handle request to the /query endpoint using Responses API.\n\nThis is a wrapper around query_endpoint_handler_base that provides\nthe Responses API specific retrieve_response and get_topic_summary functions.\n\nReturns:\n QueryResponse: Contains the conversation ID and the LLM-generated response.", | ||
| "operationId": "query_endpoint_handler_v2_v2_query_post", | ||
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/QueryRequest" | ||
| } | ||
| } | ||
| }, | ||
| "required": true | ||
| }, | ||
| "responses": { | ||
| "200": { | ||
| "description": "Successful Response", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/QueryResponse" | ||
| } | ||
| } | ||
| }, | ||
| "conversation_id": "123e4567-e89b-12d3-a456-426614174000", | ||
| "response": "LLM answer", | ||
| "referenced_documents": [ | ||
| { | ||
| "doc_url": "https://docs.openshift.com/container-platform/4.15/operators/olm/index.html", | ||
| "doc_title": "Operator Lifecycle Manager (OLM)" | ||
| } | ||
| ] | ||
| }, | ||
| "400": { | ||
| "description": "Missing or invalid credentials provided by client", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/UnauthorizedResponse" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "403": { | ||
| "description": "User is not authorized", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ForbiddenResponse" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal Server Error", | ||
| "detail": { | ||
| "response": "Unable to connect to Llama Stack", | ||
| "cause": "Connection error." | ||
| } | ||
| }, | ||
| "422": { | ||
| "description": "Validation Error", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/HTTPValidationError" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
/v2/query uses 400 for auth and lacks 404; align with v1/query.
v2 currently returns 400 for auth errors and has no 404 for missing model/provider. For consistency and correctness, switch to 401 for auth failures and add 404 Not Found with a schema.
Apply this diff to responses under /v2/query POST:
"responses": {
"200": { ... },
- "400": {
- "description": "Missing or invalid credentials provided by client",
- "content": {
- "application/json": {
- "schema": { "$ref": "#/components/schemas/UnauthorizedResponse" }
- }
- }
- },
+ "401": {
+ "description": "Missing or invalid credentials provided by client",
+ "content": {
+ "application/json": {
+ "schema": { "$ref": "#/components/schemas/UnauthorizedResponse" }
+ }
+ }
+ },
"403": {
"description": "User is not authorized",
"content": {
"application/json": {
"schema": { "$ref": "#/components/schemas/ForbiddenResponse" }
}
}
},
+ "404": {
+ "description": "Requested model or provider not found",
+ "content": {
+ "application/json": {
+ "schema": { "$ref": "#/components/schemas/NotFoundResponse" }
+ }
+ }
+ },
"500": { ... },
"422": { ... }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/openapi.json around lines 1215 to 1291, the /v2/query POST responses use
400 for auth errors and lack a 404; update the responses to match v1/query by
replacing the 400 auth response with a 401 Unauthorized response that uses the
existing UnauthorizedResponse schema, and add a 404 Not Found response entry
that references the appropriate NotFoundResponse (or equivalent) schema to
represent missing model/provider; keep other response entries unchanged.
3e15433 to
c5bcb25
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/features/query.feature (1)
62-92: Remove duplicate Background steps from scenarios.Lines 63-65, 78-80, and 91-92 repeat steps already defined in the Background section (lines 4-6). According to Gherkin best practices, Background steps apply to all scenarios in the feature and should not be repeated.
Apply this diff to remove the redundant steps:
- Scenario: Check if LLM responds to sent question with error when authenticated with invalid token - Given The service is started locally - And REST API service prefix is /v1 - Given The system is in default state + Scenario: Check if LLM responds to sent question with error when authenticated with invalid token + Given The system is in default state- Scenario: Check if LLM responds to sent question with error when model does not exist - Given The service is started locally - And REST API service prefix is /v1 - Given The system is in default state + Scenario: Check if LLM responds to sent question with error when model does not exist + Given The system is in default stateScenario: Check if LLM responds to sent question with error when attempting to access conversation - Given The service is started locally - And REST API service prefix is /v1 - Given The system is in default state + Given The system is in default state
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/openapi.json(2 hunks)src/app/endpoints/query.py(4 hunks)src/authentication/jwk_token.py(1 hunks)src/authentication/utils.py(1 hunks)tests/e2e/config/no-models-run.yaml(1 hunks)tests/e2e/features/environment.py(1 hunks)tests/e2e/features/query.feature(2 hunks)tests/e2e/features/streaming_query.feature(1 hunks)tests/integration/test_openapi_json.py(2 hunks)tests/unit/app/endpoints/test_authorized.py(1 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/authentication/test_jwk_token.py(1 hunks)tests/unit/authentication/test_noop_with_token.py(2 hunks)tests/unit/authentication/test_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/openapi.json
- tests/unit/app/endpoints/test_query.py
- tests/unit/authentication/test_jwk_token.py
- tests/integration/test_openapi_json.py
- tests/unit/app/endpoints/test_authorized.py
- tests/unit/authentication/test_utils.py
- tests/e2e/features/environment.py
- tests/e2e/features/streaming_query.feature
- tests/unit/authentication/test_noop_with_token.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/e2e/features/query.featuretests/e2e/config/no-models-run.yaml
🧬 Code graph analysis (1)
src/app/endpoints/query.py (1)
src/models/responses.py (2)
UnauthorizedResponse(1094-1117)ForbiddenResponse(1120-1142)
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (9)
src/authentication/jwk_token.py (1)
143-152: LGTM! Correct status codes for authentication failures.The change from 400 to 401 for
DecodeErrorandJoseErroris semantically correct. These exceptions represent authentication failures (inability to decode or process the JWT token), not malformed requests.src/authentication/utils.py (1)
18-24: LGTM! Correct status codes for missing credentials.The change from 400 to 401 for missing or malformed Authorization headers correctly follows RFC 7235 semantics. Missing authentication credentials should return 401 Unauthorized, not 400 Bad Request.
tests/e2e/config/no-models-run.yaml (1)
1-30: LGTM! Configuration correctly defines a no-models scenario.The configuration appropriately simulates a runtime with no available models by setting empty
model_name,model_provider, and an emptymodelslist. This supports the new@no_modelstest scenario and follows the established pattern of using tags to swap configurations during E2E testing.Based on learnings
src/app/endpoints/query.py (4)
83-93: LGTM! Response schema correctly documents new status codes.The response schema updates appropriately document:
- 401 for authentication/authorization failures
- 404 for missing model or provider resources
This aligns with HTTP semantics and the broader PR changes.
492-502: LGTM! Early guard prevents proceeding with empty models list.The early check for an empty models list correctly raises HTTP 404 with message "No models available", which matches the E2E test expectations. Failing fast here prevents downstream errors and provides clear feedback.
533-541: LGTM! Correct status code for missing model scenario.The change from 400 to 404 is semantically correct when no suitable LLM model is found. The error message "No models available" matches E2E test expectations.
553-558: LGTM! Correct status code for model/provider not found.The change from 400 to 404 correctly represents that the requested model/provider combination is not found in the available models list. This follows REST API conventions where 404 indicates the requested resource does not exist.
tests/e2e/features/query.feature (2)
56-60: LGTM! Status code change aligns with implementation.The change from 400 to 401 for missing Authorization header correctly reflects the updated authentication error handling in the codebase.
172-181: LGTM! Good coverage for no-models scenario.The new
@no_modelsscenario correctly tests the 404 response when no models are configured. The expected error message "No models available" matches the implementation and will trigger the appropriate test configuration based on the tag.Based on learnings
| @@ -0,0 +1,30 @@ | |||
| --- | |||
There was a problem hiding this comment.
This is not valid config for lightspeed-stack.
| And The body of the response is the following | ||
| """ | ||
| {"detail":"Invalid token: decode error"} | ||
| """ |
There was a problem hiding this comment.
The above scenario fails (returns 200) because "Bearer invalid" is actually a valid token - everything that matches "Bearer something" is valid token.
There was a problem hiding this comment.
if you actually want to test invalid bearer token, write something like BearerInvalid, Invalid or whatever you want. When you use Bearer with anything, it always passes the check
tests/e2e/features/query.feature
Outdated
| {"query": "Write a simple code for reversing string"} | ||
| """ | ||
| Then The status code of the response is 404 | ||
| And The body of the response contains No models available |
There was a problem hiding this comment.
The above scenario fails because, for the OpenAI provider, all its models are available by default even if the models list is empty. This behavior is specific to OpenAI and is unaffected by the model configuration.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/features/query.feature(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (3)
tests/e2e/features/query.feature (3)
56-56: LGTM! Correct status code for authentication failure.The change from 400 to 401 for missing authentication aligns with HTTP standards, where 401 Unauthorized is the appropriate status for authentication failures.
77-88: LGTM! Correct status code for missing model/provider.The scenario correctly tests that non-existent model/provider combinations return 404 Not Found, which is the appropriate HTTP status for missing resources. The error message clearly identifies the missing resource.
66-75: No issues found; implementation correctly handles invalid Bearer tokens.The authentication implementation properly validates Bearer tokens and rejects malformed tokens like "Bearer invalid" with a 401 status and decode error message. Token extraction correctly strips the Bearer prefix, and jwt.decode raises DecodeError for invalid token formats, which is caught and returns the expected response.
| Given The service is started locally | ||
| And REST API service prefix is /v1 |
There was a problem hiding this comment.
Remove redundant Background steps.
Lines 63-64 duplicate the Background steps already defined at lines 4-6. The Background section automatically applies to all scenarios in this feature, so these lines are unnecessary.
Apply this diff to remove the redundant lines:
Scenario: Check if LLM responds to sent question with error when authenticated with invalid token
- Given The service is started locally
- And REST API service prefix is /v1
Given The system is in default state📝 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.
| Given The service is started locally | |
| And REST API service prefix is /v1 | |
| Scenario: Check if LLM responds to sent question with error when authenticated with invalid token | |
| Given The system is in default state |
🤖 Prompt for AI Agents
In tests/e2e/features/query.feature around lines 63 to 64, the two Given/And
steps duplicate the Background steps defined at lines 4-6; remove these
redundant lines so scenarios rely on the Background context instead of repeating
"The service is started locally" and "REST API service prefix is /v1".
| Given The service is started locally | ||
| And REST API service prefix is /v1 |
There was a problem hiding this comment.
Remove redundant Background steps.
Lines 78-79 duplicate the Background steps already defined at lines 4-6. These are unnecessary and should be removed.
Apply this diff:
Scenario: Check if LLM responds to sent question with error when model does not exist
- Given The service is started locally
- And REST API service prefix is /v1
Given The system is in default state📝 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.
| Given The service is started locally | |
| And REST API service prefix is /v1 | |
| Scenario: Check if LLM responds to sent question with error when model does not exist | |
| Given The system is in default state |
🤖 Prompt for AI Agents
In tests/e2e/features/query.feature around lines 78 to 79, the two Given steps
"The service is started locally" and "REST API service prefix is /v1" duplicate
Background steps defined earlier (lines 4-6); remove these redundant lines so
the feature file relies on the existing Background block and avoid duplicate
setup steps.
| Given The service is started locally | ||
| And REST API service prefix is /v1 |
There was a problem hiding this comment.
Remove redundant Background steps.
Lines 91-92 duplicate the Background steps already defined at lines 4-6 and are unnecessary.
Apply this diff:
Scenario: Check if LLM responds to sent question with error when attempting to access conversation
- Given The service is started locally
- And REST API service prefix is /v1
Given The system is in default state📝 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.
| Given The service is started locally | |
| And REST API service prefix is /v1 | |
| Scenario: Check if LLM responds to sent question with error when attempting to access conversation | |
| Given The system is in default state |
🤖 Prompt for AI Agents
In tests/e2e/features/query.feature around lines 91 to 92, the two Given/And
steps "The service is started locally" and "REST API service prefix is /v1" are
duplicates of the Background defined at lines 4-6; remove these redundant lines
so the Background handles setup for the whole feature file, leaving no repeated
startup or prefix steps in the scenario(s) at 91-92.
3 participants
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Bug Fixes
Documentation
Tests