Reflected coderabbit suggestions

Andrej Simurka · Andrej Simurka · commit ff940a9e35c1 · 2025-11-24T09:40:51.000+01:00
diff --git a/docs/openapi.json b/docs/openapi.json
@@ -2013,7 +2013,7 @@
                                         "value": {
                                             "detail": {
                                                 "cause": "Storing feedback is disabled.",
-                                                "response": "Storing feedback is disabled."
+                                                "response": "Storing feedback is disabled"
                                             }
                                         }
                                     }
@@ -5674,7 +5674,7 @@
                     {
                         "detail": {
                             "cause": "Storing feedback is disabled.",
-                            "response": "Storing feedback is disabled."
+                            "response": "Storing feedback is disabled"
                         },
                         "label": "feedback"
                     },
diff --git a/src/authentication/k8s.py b/src/authentication/k8s.py
@@ -271,9 +271,6 @@ async def __call__(self, request: Request) -> tuple[str, str, bool, str]:
             )
             response = authorization_api.create_subject_access_review(sar)
 
-            if not response.status.allowed:
-                response = ForbiddenResponse.endpoint(user_id=user_info.user.uid)
-                raise HTTPException(**response.model_dump())
         except Exception as e:
             logger.error("API exception during SubjectAccessReview: %s", e)
             response = ServiceUnavailableResponse(
@@ -282,6 +279,10 @@ async def __call__(self, request: Request) -> tuple[str, str, bool, str]:
             )
             raise HTTPException(**response.model_dump()) from e
 
+        if not response.status.allowed:
+            response = ForbiddenResponse.endpoint(user_id=user_info.user.uid)
+            raise HTTPException(**response.model_dump())
+
         return (
             user_info.user.uid,
             user_info.user.username,
diff --git a/src/authentication/utils.py b/src/authentication/utils.py
@@ -6,12 +6,16 @@
 
 
 def extract_user_token(headers: Headers) -> str:
-    """Extract the bearer token from an HTTP authorization header.
+    """Extract the bearer token from an HTTP Authorization header.
 
     Args:
-        header: The authorization header containing the token.
+        headers: The request headers containing the Authorization value.
+
     Returns:
-        The extracted token if present, else an empty string.
+        The extracted bearer token.
+
+    Raises:
+        HTTPException: If the Authorization header is missing or malformed.
     """
     authorization_header = headers.get("Authorization")
     if not authorization_header:
diff --git a/src/authorization/middleware.py b/src/authorization/middleware.py
@@ -90,7 +90,7 @@ async def _perform_authorization_check(
     user_roles = await role_resolver.resolve_roles(auth) | everyone_roles
 
     if not access_resolver.check_access(action, user_roles):
-        response = ForbiddenResponse.endpoint(user_id=auth[1])
+        response = ForbiddenResponse.endpoint(user_id=auth[0])
         raise HTTPException(**response.model_dump())
 
     authorized_actions = access_resolver.get_actions(user_roles)
diff --git a/src/metrics/utils.py b/src/metrics/utils.py
@@ -6,7 +6,7 @@
 from llama_stack.models.llama.datatypes import RawMessage
 from llama_stack.models.llama.llama3.chat_format import ChatFormat
 from llama_stack.models.llama.llama3.tokenizer import Tokenizer
-from llama_stack_client import APIConnectionError
+from llama_stack_client import APIConnectionError, APIStatusError
 from llama_stack_client.types.agents.turn import Turn
 
 import metrics
@@ -27,7 +27,7 @@ async def setup_model_metrics() -> None:
     check_configuration_loaded(configuration)
     try:
         model_list = await AsyncLlamaStackClientHolder().get_client().models.list()
-    except APIConnectionError as e:
+    except (APIConnectionError, APIStatusError) as e:
         response = ServiceUnavailableResponse(backend_name="Llama Stack", cause=str(e))
         raise HTTPException(**response.model_dump()) from e
 
diff --git a/src/models/responses.py b/src/models/responses.py
@@ -1287,7 +1287,7 @@ class ForbiddenResponse(AbstractErrorResponse):
                 {
                     "label": "feedback",
                     "detail": {
-                        "response": "Storing feedback is disabled.",
+                        "response": "Storing feedback is disabled",
                         "cause": "Storing feedback is disabled.",
                     },
                 },
@@ -1332,7 +1332,7 @@ def endpoint(cls, user_id: str) -> "ForbiddenResponse":
     def feedback_disabled(cls) -> "ForbiddenResponse":
         """Create a ForbiddenResponse for disabled feedback."""
         return cls(
-            response="Feedback is disabled",
+            response="Storing feedback is disabled",
             cause="Storing feedback is disabled.",
         )
 
diff --git a/tests/e2e/features/feedback.feature b/tests/e2e/features/feedback.feature
@@ -109,7 +109,7 @@ Feature: feedback endpoint API tests
         """
         {
             "detail": {
-                "response": "Feedback is disabled",
+                "response": "Storing feedback is disabled",
                 "cause": "Storing feedback is disabled."
             }
         }  
diff --git a/tests/unit/app/endpoints/test_config.py b/tests/unit/app/endpoints/test_config.py
@@ -18,6 +18,7 @@ async def test_config_endpoint_handler_configuration_not_loaded(
     mock_authorization_resolvers(mocker)
 
     mock_config = AppConfig()
+    mock_config._configuration = None  # pylint: disable=protected-access
     mocker.patch("app.endpoints.config.configuration", mock_config)
 
     # HTTP request mock required by URL endpoint handler
diff --git a/tests/unit/app/endpoints/test_feedback.py b/tests/unit/app/endpoints/test_feedback.py
@@ -62,7 +62,7 @@ async def test_assert_feedback_enabled_disabled(mocker: MockerFixture) -> None:
         await assert_feedback_enabled(mocker.Mock())
 
     assert exc_info.value.status_code == status.HTTP_403_FORBIDDEN
-    assert exc_info.value.detail["response"] == "Feedback is disabled"  # type: ignore
+    assert exc_info.value.detail["response"] == "Storing feedback is disabled"  # type: ignore
     assert exc_info.value.detail["cause"] == "Storing feedback is disabled."  # type: ignore
 
 
diff --git a/tests/unit/app/endpoints/test_providers.py b/tests/unit/app/endpoints/test_providers.py
@@ -39,7 +39,7 @@ async def test_providers_endpoint_configuration_not_loaded(
 async def test_providers_endpoint_connection_error(
     mocker: MockerFixture, minimal_config: AppConfig
 ) -> None:
-    """Test that /providers endpoint raises HTTP 500 if Llama Stack connection fails."""
+    """Test that /providers endpoint raises HTTP 503 if Llama Stack connection fails."""
     mocker.patch("app.endpoints.providers.configuration", minimal_config)
 
     mocker.patch(
diff --git a/tests/unit/app/endpoints/test_shields.py b/tests/unit/app/endpoints/test_shields.py
@@ -23,6 +23,7 @@ async def test_shields_endpoint_handler_configuration_not_loaded(
 
     # simulate state when no configuration is loaded
     mock_config = AppConfig()
+    mock_config._configuration = None  # pylint: disable=protected-access
     mocker.patch("app.endpoints.shields.configuration", mock_config)
 
     request = Request(
diff --git a/tests/unit/app/endpoints/test_streaming_query.py b/tests/unit/app/endpoints/test_streaming_query.py
@@ -151,6 +151,7 @@ async def test_streaming_query_endpoint_handler_configuration_not_loaded(
     """Test the streaming query endpoint handler if configuration is not loaded."""
     # simulate state when no configuration is loaded
     mock_config = AppConfig()
+    mock_config._configuration = None  # pylint: disable=protected-access
     mocker.patch("app.endpoints.streaming_query.configuration", mock_config)
     # Mock authorization resolvers to avoid accessing configuration properties
     mock_authorization_resolvers(mocker)
diff --git a/tests/unit/authentication/test_noop_with_token.py b/tests/unit/authentication/test_noop_with_token.py
@@ -64,8 +64,8 @@ async def test_noop_with_token_auth_dependency_no_token() -> None:
     Test that NoopWithTokenAuthDependency raises an HTTPException when no
     Authorization header is present in the request.
 
-    Asserts that the exception has a status code of 400 and the detail message
-    "No Authorization header found".
+    Asserts that the exception has a status code of 401 and a structured
+    detail message indicating that no Authorization header was found.
     """
     dependency = NoopWithTokenAuthDependency()
 
diff --git a/tests/unit/authentication/test_utils.py b/tests/unit/authentication/test_utils.py
@@ -21,10 +21,11 @@ def test_extract_user_token_no_header() -> None:
         extract_user_token(headers)
     except HTTPException as exc:
         assert exc.status_code == 401
-        assert exc.detail["response"] == (  # type: ignore
+        detail = cast(dict[str, str], exc.detail)
+        assert detail["response"] == (
             "Missing or invalid credentials provided by client"
         )
-        assert exc.detail["cause"] == "No Authorization header found"  # type: ignore
+        assert detail["cause"] == "No Authorization header found"
 
 
 def test_extract_user_token_invalid_format() -> None:
diff --git a/tests/unit/models/responses/test_error_responses.py b/tests/unit/models/responses/test_error_responses.py
@@ -207,7 +207,7 @@ def test_factory_feedback_disabled(self) -> None:
         assert isinstance(response, AbstractErrorResponse)
         assert response.status_code == status.HTTP_403_FORBIDDEN
         assert isinstance(response.detail, DetailModel)
-        assert response.detail.response == "Feedback is disabled"
+        assert response.detail.response == "Storing feedback is disabled"
         assert response.detail.cause == "Storing feedback is disabled."
 
     def test_openapi_response(self) -> None:
@@ -238,6 +238,10 @@ def test_openapi_response(self) -> None:
         assert "detail" in feedback_example["value"]
         assert (
             feedback_example["value"]["detail"]["response"]
+            == "Storing feedback is disabled"
+        )
+        assert (
+            feedback_example["value"]["detail"]["cause"]
             == "Storing feedback is disabled."
         )
 

Original file line number	Diff line number	Diff line change
`@@ -2013,7 +2013,7 @@`
`2013`	`2013`	`"value": {`
`2014`	`2014`	`"detail": {`
`2015`	`2015`	`"cause": "Storing feedback is disabled.",`
`2016`		`- "response": "Storing feedback is disabled."`
	`2016`	`+ "response": "Storing feedback is disabled"`
`2017`	`2017`	`}`
`2018`	`2018`	`}`
`2019`	`2019`	`}`
`@@ -5674,7 +5674,7 @@`
`5674`	`5674`	`{`
`5675`	`5675`	`"detail": {`
`5676`	`5676`	`"cause": "Storing feedback is disabled.",`
`5677`		`- "response": "Storing feedback is disabled."`
	`5677`	`+ "response": "Storing feedback is disabled"`
`5678`	`5678`	`},`
`5679`	`5679`	`"label": "feedback"`
`5680`	`5680`	`},`
Original file line number	Diff line number	Diff line change
`@@ -109,7 +109,7 @@ Feature: feedback endpoint API tests`
`109`	`109`	`"""`
`110`	`110`	`{`
`111`	`111`	`"detail": {`
`112`		`- "response": "Feedback is disabled",`
	`112`	`+ "response": "Storing feedback is disabled",`
`113`	`113`	`"cause": "Storing feedback is disabled."`
`114`	`114`	`}`
`115`	`115`	`}`