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
3 changes: 3 additions & 0 deletions tests/unit/app/endpoints/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ Unit tests for the /query (v2) REST API endpoint using Responses API.
## [test_rags.py](test_rags.py)
Unit tests for the /rags REST API endpoints.

## [test_rlsapi_v1.py](test_rlsapi_v1.py)
Unit tests for the rlsapi v1 /infer REST API endpoint.

## [test_root.py](test_root.py)
Unit tests for the / endpoint handler.

Expand Down
14 changes: 13 additions & 1 deletion tests/unit/app/endpoints/test_authorized.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,19 @@ async def test_authorized_unauthorized() -> None:

@pytest.mark.asyncio
async def test_authorized_dependency_unauthorized() -> None:
"""Test that auth dependency raises HTTPException with 403 for unauthorized access."""
"""Test that auth dependency raises HTTPException with 403 for unauthorized access.

Verify extract_user_token raises HTTPException with status code 401 and the
expected detail for missing or malformed Authorization headers.

Checks two scenarios:
- Missing Authorization header: HTTPException.status_code == 401,
detail["response"] == "Missing or invalid credentials provided by
client", detail["cause"] == "No Authorization header found".
- Invalid Authorization format: HTTPException.status_code == 401,
detail["response"] == "Missing or invalid credentials provided by
client", detail["cause"] == "No token found in Authorization header".
"""
Comment on lines +50 to +62
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 17, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Correct the HTTP status code in the docstring subject line.

The docstring subject line (Line 50) states "HTTPException with 403" but the test body correctly verifies status code 401 (Lines 69, 80). The detailed docstring description and assertions are correct—this is an authentication failure scenario (missing or malformed Authorization headers) which should return 401 Unauthorized, not 403 Forbidden.

Apply this diff to fix the docstring:

-    """Test that auth dependency raises HTTPException with 403 for unauthorized access.
+    """Test that auth dependency raises HTTPException with 401 for unauthorized access.
📝 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
"""Test that auth dependency raises HTTPException with 403 for unauthorized access.
Verify extract_user_token raises HTTPException with status code 401 and the
expected detail for missing or malformed Authorization headers.
Checks two scenarios:
- Missing Authorization header: HTTPException.status_code == 401,
detail["response"] == "Missing or invalid credentials provided by
client", detail["cause"] == "No Authorization header found".
- Invalid Authorization format: HTTPException.status_code == 401,
detail["response"] == "Missing or invalid credentials provided by
client", detail["cause"] == "No token found in Authorization header".
"""
"""Test that auth dependency raises HTTPException with 401 for unauthorized access.
Verify extract_user_token raises HTTPException with status code 401 and the
expected detail for missing or malformed Authorization headers.
Checks two scenarios:
- Missing Authorization header: HTTPException.status_code == 401,
detail["response"] == "Missing or invalid credentials provided by
client", detail["cause"] == "No Authorization header found".
- Invalid Authorization format: HTTPException.status_code == 401,
detail["response"] == "Missing or invalid credentials provided by
client", detail["cause"] == "No token found in Authorization header".
"""
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_authorized.py around lines 50 to 62, the
docstring's first line incorrectly states "HTTPException with 403" while the
test assertions expect 401; update the docstring subject line to state
"HTTPException with 401" (or "401 Unauthorized") so it matches the rest of the
description and assertions about missing or malformed Authorization headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coderabbitai create issue

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
# Create a GitHub issue for the docstring error

gh issue create \
  --repo lightspeed-core/lightspeed-stack \
  --title "Fix incorrect HTTP status code in test_authorized_dependency_unauthorized docstring" \
  --body "## Description

The docstring for the \`test_authorized_dependency_unauthorized\` test function in \`tests/unit/app/endpoints/test_authorized.py\` (line 50) incorrectly states that the test verifies \"HTTPException with 403\" when it actually verifies status code 401.

## Details

- **File**: \`tests/unit/app/endpoints/test_authorized.py\`
- **Line**: 50
- **Current**: \"Test that auth dependency raises HTTPException with 403 for unauthorized access.\"
- **Expected**: \"Test that auth dependency raises HTTPException with 401 for unauthorized access.\"

The test body correctly asserts status code 401 (lines 69 and 80), which is appropriate for authentication failures (missing or malformed Authorization headers). The docstring summary line needs to be updated to match.

## Context

- **PR**: #930
- **Comment**: https://github.com/lightspeed-core/lightspeed-stack/pull/930#discussion_r2626057727
- **Requested by**: @tisnik" \
  --assignee tisnik

Length of output: 62


@tisnik I've created issue #931 for the incorrect HTTP status code in the docstring: #931

The issue has been assigned to you and includes backlinks to this PR and the review comment.


🧬 Code Graph Analysis Results

src/app/endpoints/authorized.py

  • Lines: 25-41
  • Full snippet:
async def authorized_endpoint_handler(
    auth: Annotated[AuthTuple, Depends(get_auth_dependency())],
) -> AuthorizedResponse:
    """
    Handle request to the /authorized endpoint.

    Process POST requests to the /authorized endpoint, returning
    the authenticated user's ID and username.

    Returns:
        AuthorizedResponse: Contains the user ID and username of the authenticated user.
    """
    # Ignore the user token, we should not return it in the response
    user_id, user_name, skip_userid_check, _ = auth
    return AuthorizedResponse(
        user_id=user_id, username=user_name, skip_userid_check=skip_userid_check
    )

src/authentication/utils.py

  • Lines: 8-30
  • Full snippet:
def extract_user_token(headers: Headers) -> str:
    """Extract the bearer token from an HTTP Authorization header.

    Args:
        headers: The request headers containing the Authorization value.

    Returns:
        The extracted bearer token.

    Raises:
        HTTPException: If the Authorization header is missing or malformed.
    """
    authorization_header = headers.get("Authorization")
    if not authorization_header:
        response = UnauthorizedResponse(cause="No Authorization header found")
        raise HTTPException(**response.model_dump())

    scheme_and_token = authorization_header.strip().split()
    if len(scheme_and_token) != 2 or scheme_and_token[0].lower() != "bearer":
        response = UnauthorizedResponse(cause="No token found in Authorization header")
        raise HTTPException(**response.model_dump())

    return scheme_and_token[1]

tests/unit/app/endpoints/test_authorized.py

  • Size/Scope: Full test file contents (long)
  • Summary description:
    • Tests for the /authorized endpoint behavior, including:
      • Successful handling returning user_id, username, and skip_userid_check.
      • Unauthorized scenarios where the handler is invoked with missing or malformed auth data, triggering TypeError or ValueError.
      • A test verifying that extract_user_token raises HTTPException with 401 status and specific detail content for missing header or invalid Authorization format.

🧠 Learnings used
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/app/endpoints/**/*.py : Use FastAPI `HTTPException` with appropriate status codes for API endpoint error handling

# Test the auth utility function that would be called by auth dependencies
# This simulates the unauthorized scenario that would prevent the handler from being called

Expand Down
119 changes: 111 additions & 8 deletions tests/unit/app/endpoints/test_conversations.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,13 @@

@pytest.fixture
def dummy_request() -> Request:
"""Mock request object for testing."""
"""Mock request object for testing.

Create a mock FastAPI Request configured for tests with full authorization.

The returned Request has state.authorized_actions set to a set containing
every member of Action.
"""
request = Request(
scope={
"type": "http",
Expand All @@ -56,7 +62,35 @@ def create_mock_conversation(
last_used_provider: str,
topic_summary: Optional[str] = None,
) -> MockType:
"""Helper function to create a mock conversation object with all required attributes."""
"""Helper function to create a mock conversation object with all required attributes.

Create a mock conversation object with the attributes used by the
conversations list and detail tests.

The returned mock has the following attributes:
- id: the conversation identifier (string)
- created_at.isoformat(): returns the provided created_at string
- last_message_at.isoformat(): returns the provided last_message_at string
- message_count: number of messages in the conversation
- last_used_model: model identifier last used in the conversation
- last_used_provider: provider identifier last used in the conversation
- topic_summary: optional topic summary (may be None or empty string)

Parameters:
mocker (MockerFixture): pytest mocker fixture used to build the mock object.
conversation_id (str): Conversation identifier to assign to the mock.
created_at (str): ISO-formatted created-at timestamp to be returned by
created_at.isoformat().
last_message_at (str): ISO-formatted last-message timestamp to be
returned by last_message_at.isoformat().
message_count (int): Message count to assign to the mock.
last_used_model (str): Last used model string to assign to the mock.
last_used_provider (str): Last used provider string to assign to the mock.
topic_summary (Optional[str]): Optional topic summary to assign to the mock.

Returns:
mock_conversation: A mock object configured with the above attributes.
"""
mock_conversation = mocker.Mock()
mock_conversation.id = conversation_id
mock_conversation.created_at = mocker.Mock()
Expand All @@ -73,7 +107,20 @@ def create_mock_conversation(
def mock_database_session(
mocker: MockerFixture, query_result: Optional[list[MockType]] = None
) -> MockType:
"""Helper function to mock get_session with proper context manager support."""
"""Helper function to mock get_session with proper context manager support.

Create and patch a mocked database session and a context-manager-compatible get_session.

Parameters:
mocker (pytest.MockerFixture): Fixture used to create and patch mocks.
query_result (Optional[list]): If provided, configures the
session.query().all() and session.query().filter_by().all() to return
this list.

Returns:
Mock: The mocked session object that will be yielded by the patched
get_session context manager.
"""
mock_session = mocker.Mock()
if query_result is not None:
# Mock both the filtered and unfiltered query paths
Expand All @@ -94,7 +141,16 @@ def mock_database_session(

@pytest.fixture(name="setup_configuration")
def setup_configuration_fixture() -> AppConfig:
"""Set up configuration for tests."""
"""Set up configuration for tests.

Create an AppConfig prepopulated with test-friendly default settings.

Returns:
AppConfig: An AppConfig instance initialized from a dictionary
containing defaults suitable for tests (local service host/port,
disabled auth and user-data collection, test Llama Stack API key and
URL, and single worker).
"""
config_dict: dict[str, Any] = {
"name": "test",
"service": {
Expand Down Expand Up @@ -123,7 +179,29 @@ def setup_configuration_fixture() -> AppConfig:

@pytest.fixture(name="mock_session_data")
def mock_session_data_fixture() -> dict[str, Any]:
"""Create mock session data for testing."""
"""Create mock session data for testing.

Provide a representative mock session data payload used by tests to
simulate a conversation session.

The returned dictionary contains:
- session_id: conversation identifier.
- session_name: human-readable session name.
- started_at: ISO 8601 timestamp when the session started.
- turns: list of turn objects; each turn includes:
- turn_id: identifier for the turn.
- input_messages: list of input message objects with `content`, `role`,
and optional `context`.
- output_message: assistant response object with `content`, `role`, and
auxiliary fields (e.g., `stop_reason`, `tool_calls`) that tests
expect to be filtered by simplification logic.
- started_at / completed_at: ISO 8601 timestamps for the turn.
- steps: detailed internal steps included to verify they are removed by simplification.

Returns:
dict: A mock session data structure matching the shape produced by the
Llama Stack client for use in unit tests.
"""
return {
"session_id": VALID_CONVERSATION_ID,
"session_name": "test-session",
Expand Down Expand Up @@ -165,7 +243,17 @@ def mock_session_data_fixture() -> dict[str, Any]:

@pytest.fixture(name="expected_chat_history")
def expected_chat_history_fixture() -> list[dict[str, Any]]:
"""Create expected simplified chat history for testing."""
"""Create expected simplified chat history for testing.

Expected simplified chat history used by tests.

Returns:
list[dict[str, Any]]: A list of conversation turns. Each turn contains:
- messages: list of message dicts with `content` (str) and `type`
(`"user"` or `"assistant"`)
- started_at: ISO 8601 UTC timestamp string for the turn start
- completed_at: ISO 8601 UTC timestamp string for the turn end
"""
return [
{
"messages": [
Expand All @@ -188,7 +276,14 @@ def expected_chat_history_fixture() -> list[dict[str, Any]]:

@pytest.fixture(name="mock_conversation")
def mock_conversation_fixture() -> UserConversation:
"""Create a mock UserConversation object for testing."""
"""Create a mock UserConversation object for testing.

Returns:
mock_conv (UserConversation): A UserConversation initialized with
VALID_CONVERSATION_ID, user_id set to "another_user", message_count 2,
last_used_model "mock-model", last_used_provider "mock-provider", and
topic_summary "Mock topic".
"""
mock_conv = UserConversation()
mock_conv.id = VALID_CONVERSATION_ID
mock_conv.user_id = "another_user" # Different from test auth
Expand Down Expand Up @@ -366,7 +461,15 @@ async def test_llama_stack_not_found_error(
setup_configuration: AppConfig,
dummy_request: Request,
) -> None:
"""Test the endpoint when LlamaStack returns NotFoundError."""
"""Test the endpoint when LlamaStack returns NotFoundError.

Verify the GET /conversations/{conversation_id} handler raises an HTTP
404 when the Llama Stack client reports the session as not found.

Asserts that the raised HTTPException contains a response message
indicating the conversation was not found and a cause that includes
"does not exist" and the conversation ID.
"""
mock_authorization_resolvers(mocker)
mocker.patch("app.endpoints.conversations.configuration", setup_configuration)
mocker.patch("app.endpoints.conversations.check_suid", return_value=True)
Expand Down
46 changes: 42 additions & 4 deletions tests/unit/app/endpoints/test_conversations_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,16 @@ def test_transform_message_with_empty_referenced_documents(self) -> None:

@pytest.fixture
def mock_configuration(mocker: MockerFixture) -> MockType:
"""Mock configuration with conversation cache."""
"""Mock configuration with conversation cache.

Create a mocked configuration object with a mocked `conversation_cache` attribute.

Parameters:
mocker (pytest.MockFixture): The pytest-mock fixture used to create mocks.

Returns:
Mock: A mock configuration object whose `conversation_cache` attribute is a mock.
"""
mock_config = mocker.Mock()
mock_cache = mocker.Mock()
mock_config.conversation_cache = mock_cache
Expand Down Expand Up @@ -303,7 +312,15 @@ async def test_successful_retrieval_empty_list(
async def test_with_skip_userid_check(
self, mocker: MockerFixture, mock_configuration: MockType
) -> None:
"""Test the endpoint with skip_userid_check flag."""
"""Test the endpoint with skip_userid_check flag.

Verify the conversations list handler forwards the skip_userid_check
flag from the auth tuple to the conversation cache.

Sets up a mocked configuration and auth tuple with the skip flag set to
True, invokes the handler, and asserts that `conversation_cache.list`
is called with the user ID and `True`.
"""
mock_authorization_resolvers(mocker)
mocker.patch("app.endpoints.conversations_v2.configuration", mock_configuration)
mock_configuration.conversation_cache.list.return_value = []
Expand Down Expand Up @@ -375,7 +392,18 @@ async def test_invalid_conversation_id_format(
async def test_conversation_cache_not_configured(
self, mocker: MockerFixture
) -> None:
"""Test the endpoint when conversation cache is not configured."""
"""Test the endpoint when conversation cache is not configured.

Verify the conversation GET endpoint raises an HTTP 500 error when the
conversation cache is not configured.

Patches the application configuration so
`conversation_cache_configuration.type` is None and ensures
`check_suid` returns True, then calls
`get_conversation_endpoint_handler` and asserts that it raises an
`HTTPException` with status code 500 and a response detail containing
"Conversation cache not configured".
"""
mock_authorization_resolvers(mocker)
mock_config = mocker.Mock()
mock_config.conversation_cache_configuration = mocker.Mock()
Expand Down Expand Up @@ -632,7 +660,17 @@ async def test_unsuccessful_deletion(
async def test_with_skip_userid_check(
self, mocker: MockerFixture, mock_configuration: MockType
) -> None:
"""Test the endpoint with skip_userid_check flag."""
"""Test the endpoint with skip_userid_check flag.

Verifies that providing an auth tuple with the skip-userid flag set
causes the conversation delete handler to call the cache delete method
with the skip flag.

This test patches configuration and SUID validation, supplies an auth
tuple where the third element is True, invokes
delete_conversation_endpoint_handler, and asserts the cache.delete was
called with (user_id, conversation_id, True).
"""
mock_authorization_resolvers(mocker)
mocker.patch("app.endpoints.conversations_v2.configuration", mock_configuration)
mocker.patch("app.endpoints.conversations_v2.check_suid", return_value=True)
Expand Down
13 changes: 12 additions & 1 deletion tests/unit/app/endpoints/test_health.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,18 @@ class TestGetProvidersHealthStatuses:
"""Test cases for the get_providers_health_statuses function."""

async def test_get_providers_health_statuses(self, mocker: MockerFixture) -> None:
"""Test get_providers_health_statuses with healthy providers."""
"""Test get_providers_health_statuses with healthy providers.

Verify get_providers_health_statuses returns a ProviderHealthStatus
entry for each provider reported by the client.

Mocks an AsyncLlamaStack client whose providers.list() returns three
providers with distinct health dicts, then asserts the function
produces three results with:
- provider1: status OK, message "All good"
- provider2: status NOT_IMPLEMENTED, message "Provider does not implement health check"
- unhealthy_provider: status ERROR, message "Connection failed"
"""
# Mock the imports
mock_lsc = mocker.patch("client.AsyncLlamaStackClientHolder.get_client")

Expand Down
13 changes: 12 additions & 1 deletion tests/unit/app/endpoints/test_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,18 @@ async def test_info_endpoint(mocker: MockerFixture) -> None:

@pytest.mark.asyncio
async def test_info_endpoint_connection_error(mocker: MockerFixture) -> None:
"""Test the info endpoint handler."""
"""Test the info endpoint handler.

Verify that info_endpoint_handler raises an HTTPException with
status 503 when the LlamaStack client cannot connect.

Sets up application configuration and patches the LlamaStack
client so that calling its version inspection raises an
APIConnectionError, then asserts the raised HTTPException has
status code 503 and a detail payload containing a "response" of
"Service unavailable" and a "cause" that includes "Unable to
connect to Llama Stack".
"""
mock_authorization_resolvers(mocker)

# configuration for tests
Expand Down
12 changes: 11 additions & 1 deletion tests/unit/app/endpoints/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,17 @@ async def test_models_endpoint_handler_configuration_not_loaded(
async def test_models_endpoint_handler_configuration_loaded(
mocker: MockerFixture,
) -> None:
"""Test the models endpoint handler if configuration is loaded."""
"""Test the models endpoint handler if configuration is loaded.

Verify the models endpoint raises HTTP 503 when configuration is loaded but
the Llama Stack client cannot connect.

Loads an AppConfig from a test dictionary, patches the endpoint's
configuration and AsyncLlamaStackClientHolder so that get_client raises
APIConnectionError, issues a request with an authorization header, and
asserts that calling the handler raises an HTTPException with status 503
and a detail response of "Unable to connect to Llama Stack".
"""
mock_authorization_resolvers(mocker)

# configuration for tests
Expand Down
Loading
Loading