LCORE-393: Providers endpoint #641
Conversation
WalkthroughAdds provider discovery endpoints: GET /v1/providers and GET /v1/providers/{provider_id}; new router registration; OpenAPI and docs updates; new ProviderResponse and ProvidersListResponse models; two Action enum members; endpoint implementations with error handling; and unit tests for success and failure cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as FastAPI (/v1)
participant EP as Providers endpoints
participant Holder as AsyncLlamaStackClientHolder
participant Stack as Llama Stack
rect rgb(240,248,255)
note over Client,EP: List providers
Client->>API: GET /v1/providers
API->>EP: providers_endpoint_handler()
EP->>Holder: get_client()
Holder->>Stack: list_providers()
alt success
Stack-->>Holder: providers[]
Holder-->>EP: providers[]
EP->>EP: group_providers()
EP-->>API: 200 ProvidersListResponse
API-->>Client: 200 JSON
else connection error
Stack-->>Holder: APIConnectionError
EP-->>API: 500 {"detail":"Unable to connect to Llama Stack"}
API-->>Client: 500
else other error
EP-->>API: 500 {"detail":"Unable to retrieve list of providers"}
API-->>Client: 500
end
end
rect rgb(245,255,245)
note over Client,EP: Get provider by id
Client->>API: GET /v1/providers/{provider_id}
API->>EP: get_provider_endpoint_handler(id)
EP->>Holder: get_client()
Holder->>Stack: list_providers()
alt found
Stack-->>Holder: providers[]
Holder-->>EP: providers[]
EP->>EP: find matching provider_id
EP-->>API: 200 ProviderResponse
API-->>Client: 200 JSON
else not found
EP-->>API: 404 {"detail":"Provider not found"}
API-->>Client: 404
else connection error
EP-->>API: 500 {"detail":"Unable to connect to Llama Stack"}
API-->>Client: 500
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
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 |
asimurka
force-pushed
the
providers_endpoint
branch
from
7199a1e to
39ea846
Compare
asimurka
force-pushed
the
providers_endpoint
branch
from
39ea846 to
f9a977f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
tests/unit/app/test_routers.py (1)
73-76: Remove redundant router assertions.The new provider assertions accidentally reintroduce the existing
root/info/modelschecks, so the same assertions now run twice. Please drop the duplicates and keep only the new provider-specific checks to keep the test focused.Apply this diff:
- assert root.router in app.get_routers() - assert info.router in app.get_routers() - assert models.router in app.get_routers() - assert providers.router in app.get_routers() + assert providers.router in app.get_routers()- assert app.get_router_prefix(root.router) == "" - assert app.get_router_prefix(info.router) == "/v1" - assert app.get_router_prefix(models.router) == "/v1" - assert app.get_router_prefix(providers.router) == "/v1" + assert app.get_router_prefix(providers.router) == "/v1"Also applies to: 101-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/auth.md(1 hunks)docs/openapi.json(3 hunks)src/app/endpoints/providers.py(1 hunks)src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(2 hunks)tests/unit/app/endpoints/test_providers.py(1 hunks)tests/unit/app/test_routers.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/models/config.pysrc/app/endpoints/providers.pysrc/models/responses.pysrc/app/routers.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/config.pysrc/app/endpoints/providers.pytests/unit/app/test_routers.pytests/unit/app/endpoints/test_providers.pysrc/models/responses.pysrc/app/routers.py
src/{models/config.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields
Files:
src/models/config.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/config.pysrc/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/config.pysrc/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/providers.pysrc/app/routers.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/providers.pysrc/app/routers.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/providers.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/app/test_routers.pytests/unit/app/endpoints/test_providers.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/unit/app/test_routers.pytests/unit/app/endpoints/test_providers.py
🧬 Code graph analysis (3)
src/app/endpoints/providers.py (7)
src/authentication/__init__.py (1)
get_auth_dependency(14-52)src/authorization/middleware.py (1)
authorize(111-122)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/configuration.py (2)
configuration(65-69)llama_stack_configuration(79-83)src/models/config.py (2)
config(139-145)Action(328-371)src/models/responses.py (2)
ProvidersListResponse(89-119)ProviderResponse(122-145)src/utils/endpoints.py (1)
check_configuration_loaded(68-80)
tests/unit/app/endpoints/test_providers.py (2)
src/app/endpoints/providers.py (2)
get_provider_endpoint_handler(157-225)providers_endpoint_handler(69-130)src/client.py (1)
get_client(49-55)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(35-50)
🔇 Additional comments (6)
src/app/endpoints/providers.py (3)
67-130: LGTM!The endpoint implementation is well-structured with:
- Proper type annotations
- Comprehensive error handling for connection and unexpected errors
- Clear separation of concerns with the group_providers helper
- Good logging practices
133-152: LGTM!The
group_providershelper function is well-implemented with:
- Clear docstring explaining the transformation
- Efficient use of
setdefault- Proper type annotations
- Clean extraction of relevant fields
162-225: LGTM!The endpoint implementation is well-structured with:
- Proper error handling including re-raising HTTPException to preserve 404 status
- Clean use of
next()to find matching provider- Comprehensive exception handling for connection and unexpected errors
- Good logging practices
docs/openapi.json (3)
214-272: LGTM!The
/v1/providers/{provider_id}endpoint definition is well-structured with:
- Proper parameter definition for the path variable
- Correct schema reference to ProviderResponse
- Complete response definitions for success (200), not found (404), and error (500) cases
- Helpful examples in the response documentation
1233-1234: LGTM!The Action enum correctly includes the new actions
list_providersandget_providerto support the new provider endpoints.
2694-2792: LGTM!The schema definitions for
ProviderResponseandProvidersListResponseare well-structured with:
- Complete property definitions with types and descriptions
- Proper required field lists
- Helpful examples that match the implementation
- Correct representation of the grouped providers structure in ProvidersListResponse
| @pytest.mark.asyncio | ||
| async def test_providers_endpoint_configuration_not_loaded(mocker): | ||
| """Test that /providers endpoint raises HTTP 500 if configuration is not loaded.""" | ||
| mocker.patch("app.endpoints.providers.configuration", None) | ||
| request = Request(scope={"type": "http"}) | ||
| auth = ("user", "token", {}) | ||
|
|
||
| with pytest.raises(HTTPException) as e: | ||
| await providers_endpoint_handler(request=request, auth=auth) | ||
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared MOCK_AUTH constant for consistency.
The coding guidelines specify using the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests. The current auth tuple ("user", "token", {}) differs in both structure (3 vs 4 elements) and values.
As per coding guidelines.
Apply this diff to align with the guideline:
+from tests.constants import MOCK_AUTH
+
@pytest.mark.asyncio
async def test_providers_endpoint_configuration_not_loaded(mocker):
"""Test that /providers endpoint raises HTTP 500 if configuration is not loaded."""
mocker.patch("app.endpoints.providers.configuration", None)
request = Request(scope={"type": "http"})
- auth = ("user", "token", {})
+ auth = MOCK_AUTH
with pytest.raises(HTTPException) as e:
await providers_endpoint_handler(request=request, auth=auth)
assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR📝 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.
| @pytest.mark.asyncio | |
| async def test_providers_endpoint_configuration_not_loaded(mocker): | |
| """Test that /providers endpoint raises HTTP 500 if configuration is not loaded.""" | |
| mocker.patch("app.endpoints.providers.configuration", None) | |
| request = Request(scope={"type": "http"}) | |
| auth = ("user", "token", {}) | |
| with pytest.raises(HTTPException) as e: | |
| await providers_endpoint_handler(request=request, auth=auth) | |
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | |
| from tests.constants import MOCK_AUTH | |
| @pytest.mark.asyncio | |
| async def test_providers_endpoint_configuration_not_loaded(mocker): | |
| """Test that /providers endpoint raises HTTP 500 if configuration is not loaded.""" | |
| mocker.patch("app.endpoints.providers.configuration", None) | |
| request = Request(scope={"type": "http"}) | |
| auth = MOCK_AUTH | |
| with pytest.raises(HTTPException) as e: | |
| await providers_endpoint_handler(request=request, auth=auth) | |
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_providers.py around lines 15 to 24, replace the
hardcoded auth tuple ("user", "token", {}) with the shared test constant
MOCK_AUTH to match the project guideline; update the test to reference MOCK_AUTH
(import it if necessary from the test utilities or conftest) so the test uses
the correct four-element mock auth ("mock_user_id", "mock_username", False,
"mock_token") and maintains consistency with other tests.
| @pytest.mark.asyncio | ||
| async def test_providers_endpoint_success(mocker): | ||
| """Test that /providers endpoint returns a grouped list of providers on success.""" | ||
| provider_list = [ | ||
| { | ||
| "api": "inference", | ||
| "provider_id": "openai", | ||
| "provider_type": "remote::openai", | ||
| }, | ||
| { | ||
| "api": "inference", | ||
| "provider_id": "st", | ||
| "provider_type": "inline::sentence-transformers", | ||
| }, | ||
| { | ||
| "api": "datasetio", | ||
| "provider_id": "huggingface", | ||
| "provider_type": "remote::huggingface", | ||
| }, | ||
| ] | ||
| mock_client = AsyncMock() | ||
| mock_client.providers.list.return_value = provider_list | ||
| mocker.patch( | ||
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | ||
| ).return_value.get_client.return_value = mock_client | ||
|
|
||
| request = Request(scope={"type": "http"}) | ||
| auth = ("user", "token", {}) | ||
|
|
||
| response = await providers_endpoint_handler(request=request, auth=auth) | ||
| assert "inference" in response.providers | ||
| assert len(response.providers["inference"]) == 2 | ||
| assert "datasetio" in response.providers |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared MOCK_AUTH constant for consistency.
Same issue - use the shared MOCK_AUTH constant.
As per coding guidelines.
Apply this diff:
@pytest.mark.asyncio
async def test_providers_endpoint_success(mocker):
"""Test that /providers endpoint returns a grouped list of providers on success."""
provider_list = [
{
"api": "inference",
"provider_id": "openai",
"provider_type": "remote::openai",
},
{
"api": "inference",
"provider_id": "st",
"provider_type": "inline::sentence-transformers",
},
{
"api": "datasetio",
"provider_id": "huggingface",
"provider_type": "remote::huggingface",
},
]
mock_client = AsyncMock()
mock_client.providers.list.return_value = provider_list
mocker.patch(
"app.endpoints.providers.AsyncLlamaStackClientHolder"
).return_value.get_client.return_value = mock_client
request = Request(scope={"type": "http"})
- auth = ("user", "token", {})
+ auth = MOCK_AUTH
response = await providers_endpoint_handler(request=request, auth=auth)
assert "inference" in response.providers
assert len(response.providers["inference"]) == 2
assert "datasetio" in response.providers📝 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.
| @pytest.mark.asyncio | |
| async def test_providers_endpoint_success(mocker): | |
| """Test that /providers endpoint returns a grouped list of providers on success.""" | |
| provider_list = [ | |
| { | |
| "api": "inference", | |
| "provider_id": "openai", | |
| "provider_type": "remote::openai", | |
| }, | |
| { | |
| "api": "inference", | |
| "provider_id": "st", | |
| "provider_type": "inline::sentence-transformers", | |
| }, | |
| { | |
| "api": "datasetio", | |
| "provider_id": "huggingface", | |
| "provider_type": "remote::huggingface", | |
| }, | |
| ] | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.return_value = provider_list | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = ("user", "token", {}) | |
| response = await providers_endpoint_handler(request=request, auth=auth) | |
| assert "inference" in response.providers | |
| assert len(response.providers["inference"]) == 2 | |
| assert "datasetio" in response.providers | |
| @pytest.mark.asyncio | |
| async def test_providers_endpoint_success(mocker): | |
| """Test that /providers endpoint returns a grouped list of providers on success.""" | |
| provider_list = [ | |
| { | |
| "api": "inference", | |
| "provider_id": "openai", | |
| "provider_type": "remote::openai", | |
| }, | |
| { | |
| "api": "inference", | |
| "provider_id": "st", | |
| "provider_type": "inline::sentence-transformers", | |
| }, | |
| { | |
| "api": "datasetio", | |
| "provider_id": "huggingface", | |
| "provider_type": "remote::huggingface", | |
| }, | |
| ] | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.return_value = provider_list | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = MOCK_AUTH | |
| response = await providers_endpoint_handler(request=request, auth=auth) | |
| assert "inference" in response.providers | |
| assert len(response.providers["inference"]) == 2 | |
| assert "datasetio" in response.providers |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_providers.py around lines 45 to 77, the test
uses a hardcoded auth tuple (auth = ("user", "token", {})); replace that with
the shared MOCK_AUTH constant to follow project conventions and ensure
consistency (i.e., set auth = MOCK_AUTH), and if MOCK_AUTH is not already
imported at the top of the file, add the appropriate import from the shared test
utilities.
| @pytest.mark.asyncio | ||
| async def test_get_provider_not_found(mocker): | ||
| """Test that /providers/{provider_id} endpoint raises HTTP 404 if the provider is not found.""" | ||
| mock_client = AsyncMock() | ||
| mock_client.providers.list.return_value = [] | ||
| mocker.patch( | ||
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | ||
| ).return_value.get_client.return_value = mock_client | ||
|
|
||
| request = Request(scope={"type": "http"}) | ||
| auth = ("user", "token", {}) | ||
|
|
||
| with pytest.raises(HTTPException) as e: | ||
| await get_provider_endpoint_handler( | ||
| request=request, provider_id="openai", auth=auth | ||
| ) | ||
| assert e.value.status_code == status.HTTP_404_NOT_FOUND | ||
| assert "not found" in e.value.detail["response"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared MOCK_AUTH constant for consistency.
Same issue - use the shared MOCK_AUTH constant.
As per coding guidelines.
Apply this diff:
@pytest.mark.asyncio
async def test_get_provider_not_found(mocker):
"""Test that /providers/{provider_id} endpoint raises HTTP 404 if the provider is not found."""
mock_client = AsyncMock()
mock_client.providers.list.return_value = []
mocker.patch(
"app.endpoints.providers.AsyncLlamaStackClientHolder"
).return_value.get_client.return_value = mock_client
request = Request(scope={"type": "http"})
- auth = ("user", "token", {})
+ auth = MOCK_AUTH
with pytest.raises(HTTPException) as e:
await get_provider_endpoint_handler(
request=request, provider_id="openai", auth=auth
)
assert e.value.status_code == status.HTTP_404_NOT_FOUND
assert "not found" in e.value.detail["response"]📝 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.
| @pytest.mark.asyncio | |
| async def test_get_provider_not_found(mocker): | |
| """Test that /providers/{provider_id} endpoint raises HTTP 404 if the provider is not found.""" | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.return_value = [] | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = ("user", "token", {}) | |
| with pytest.raises(HTTPException) as e: | |
| await get_provider_endpoint_handler( | |
| request=request, provider_id="openai", auth=auth | |
| ) | |
| assert e.value.status_code == status.HTTP_404_NOT_FOUND | |
| assert "not found" in e.value.detail["response"] | |
| @pytest.mark.asyncio | |
| async def test_get_provider_not_found(mocker): | |
| """Test that /providers/{provider_id} endpoint raises HTTP 404 if the provider is not found.""" | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.return_value = [] | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = MOCK_AUTH | |
| with pytest.raises(HTTPException) as e: | |
| await get_provider_endpoint_handler( | |
| request=request, provider_id="openai", auth=auth | |
| ) | |
| assert e.value.status_code == status.HTTP_404_NOT_FOUND | |
| assert "not found" in e.value.detail["response"] |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_providers.py around lines 80 to 97, the test
uses a local auth tuple (auth = ("user", "token", {})) instead of the shared
MOCK_AUTH constant; replace the local tuple with auth = MOCK_AUTH and add the
appropriate import for MOCK_AUTH at the top of the file (or ensure the test
module already imports it) so the test uses the shared constant per coding
guidelines.
| @pytest.mark.asyncio | ||
| async def test_get_provider_success(mocker): | ||
| """Test that /providers/{provider_id} endpoint returns provider details on success.""" | ||
| provider = { | ||
| "api": "inference", | ||
| "provider_id": "openai", | ||
| "provider_type": "remote::openai", | ||
| "config": {"api_key": "*****"}, | ||
| "health": {"status": "OK", "message": "Healthy"}, | ||
| } | ||
| mock_client = AsyncMock() | ||
| mock_client.providers.list.return_value = [provider] | ||
| mocker.patch( | ||
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | ||
| ).return_value.get_client.return_value = mock_client | ||
|
|
||
| request = Request(scope={"type": "http"}) | ||
| auth = ("user", "token", {}) | ||
|
|
||
| response = await get_provider_endpoint_handler( | ||
| request=request, provider_id="openai", auth=auth | ||
| ) | ||
| assert response.provider_id == "openai" | ||
| assert response.api == "inference" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared MOCK_AUTH constant for consistency.
Same issue - use the shared MOCK_AUTH constant.
As per coding guidelines.
Apply this diff:
@pytest.mark.asyncio
async def test_get_provider_success(mocker):
"""Test that /providers/{provider_id} endpoint returns provider details on success."""
provider = {
"api": "inference",
"provider_id": "openai",
"provider_type": "remote::openai",
"config": {"api_key": "*****"},
"health": {"status": "OK", "message": "Healthy"},
}
mock_client = AsyncMock()
mock_client.providers.list.return_value = [provider]
mocker.patch(
"app.endpoints.providers.AsyncLlamaStackClientHolder"
).return_value.get_client.return_value = mock_client
request = Request(scope={"type": "http"})
- auth = ("user", "token", {})
+ auth = MOCK_AUTH
response = await get_provider_endpoint_handler(
request=request, provider_id="openai", auth=auth
)
assert response.provider_id == "openai"
assert response.api == "inference"📝 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.
| @pytest.mark.asyncio | |
| async def test_get_provider_success(mocker): | |
| """Test that /providers/{provider_id} endpoint returns provider details on success.""" | |
| provider = { | |
| "api": "inference", | |
| "provider_id": "openai", | |
| "provider_type": "remote::openai", | |
| "config": {"api_key": "*****"}, | |
| "health": {"status": "OK", "message": "Healthy"}, | |
| } | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.return_value = [provider] | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = ("user", "token", {}) | |
| response = await get_provider_endpoint_handler( | |
| request=request, provider_id="openai", auth=auth | |
| ) | |
| assert response.provider_id == "openai" | |
| assert response.api == "inference" | |
| @pytest.mark.asyncio | |
| async def test_get_provider_success(mocker): | |
| """Test that /providers/{provider_id} endpoint returns provider details on success.""" | |
| provider = { | |
| "api": "inference", | |
| "provider_id": "openai", | |
| "provider_type": "remote::openai", | |
| "config": {"api_key": "*****"}, | |
| "health": {"status": "OK", "message": "Healthy"}, | |
| } | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.return_value = [provider] | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = MOCK_AUTH | |
| response = await get_provider_endpoint_handler( | |
| request=request, provider_id="openai", auth=auth | |
| ) | |
| assert response.provider_id == "openai" | |
| assert response.api == "inference" |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_providers.py around lines 100 to 123, the test
uses a local auth tuple (auth = ("user", "token", {})) instead of the shared
MOCK_AUTH constant; replace the local auth assignment and any usage of that
variable with the shared MOCK_AUTH constant (ensure the test imports or has
access to MOCK_AUTH at top of the file if not already) so the test uses the
standardized mock authentication value per coding guidelines.
| @pytest.mark.asyncio | ||
| async def test_get_provider_connection_error(mocker): | ||
| """Test that /providers/{provider_id} raises HTTP 500 if Llama Stack connection fails.""" | ||
| mock_client = AsyncMock() | ||
| mock_client.providers.list.side_effect = APIConnectionError(request=None) | ||
| mocker.patch( | ||
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | ||
| ).return_value.get_client.return_value = mock_client | ||
|
|
||
| request = Request(scope={"type": "http"}) | ||
| auth = ("user", "token", {}) | ||
|
|
||
| with pytest.raises(HTTPException) as e: | ||
| await get_provider_endpoint_handler( | ||
| request=request, provider_id="openai", auth=auth | ||
| ) | ||
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | ||
| assert "Unable to connect to Llama Stack" in e.value.detail["response"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared MOCK_AUTH constant for consistency.
Same issue - use the shared MOCK_AUTH constant.
As per coding guidelines.
Apply this diff:
@pytest.mark.asyncio
async def test_get_provider_connection_error(mocker):
"""Test that /providers/{provider_id} raises HTTP 500 if Llama Stack connection fails."""
mock_client = AsyncMock()
mock_client.providers.list.side_effect = APIConnectionError(request=None)
mocker.patch(
"app.endpoints.providers.AsyncLlamaStackClientHolder"
).return_value.get_client.return_value = mock_client
request = Request(scope={"type": "http"})
- auth = ("user", "token", {})
+ auth = MOCK_AUTH
with pytest.raises(HTTPException) as e:
await get_provider_endpoint_handler(
request=request, provider_id="openai", auth=auth
)
assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
assert "Unable to connect to Llama Stack" in e.value.detail["response"]📝 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.
| @pytest.mark.asyncio | |
| async def test_get_provider_connection_error(mocker): | |
| """Test that /providers/{provider_id} raises HTTP 500 if Llama Stack connection fails.""" | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.side_effect = APIConnectionError(request=None) | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = ("user", "token", {}) | |
| with pytest.raises(HTTPException) as e: | |
| await get_provider_endpoint_handler( | |
| request=request, provider_id="openai", auth=auth | |
| ) | |
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | |
| assert "Unable to connect to Llama Stack" in e.value.detail["response"] | |
| @pytest.mark.asyncio | |
| async def test_get_provider_connection_error(mocker): | |
| """Test that /providers/{provider_id} raises HTTP 500 if Llama Stack connection fails.""" | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.side_effect = APIConnectionError(request=None) | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = MOCK_AUTH | |
| with pytest.raises(HTTPException) as e: | |
| await get_provider_endpoint_handler( | |
| request=request, provider_id="openai", auth=auth | |
| ) | |
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | |
| assert "Unable to connect to Llama Stack" in e.value.detail["response"] |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_providers.py around lines 126 to 143, the test
uses a local auth tuple ("user", "token", {}) instead of the shared MOCK_AUTH
constant; replace the local auth assignment and use MOCK_AUTH when calling
get_provider_endpoint_handler so the test uses the standardized mock credentials
consistent with other tests (ensure the existing MOCK_AUTH import/definition in
the module is used).
| @pytest.mark.asyncio | ||
| async def test_get_provider_unexpected_exception(mocker): | ||
| """Test that /providers/{provider_id} endpoint raises HTTP 500 for unexpected exceptions.""" | ||
| mock_client = AsyncMock() | ||
| mock_client.providers.list.side_effect = Exception("boom") | ||
| mocker.patch( | ||
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | ||
| ).return_value.get_client.return_value = mock_client | ||
|
|
||
| request = Request(scope={"type": "http"}) | ||
| auth = ("user", "token", {}) | ||
|
|
||
| with pytest.raises(HTTPException) as e: | ||
| await get_provider_endpoint_handler( | ||
| request=request, provider_id="openai", auth=auth | ||
| ) | ||
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | ||
| assert "Unable to retrieve list of providers" in e.value.detail["response"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared MOCK_AUTH constant for consistency.
Same issue - use the shared MOCK_AUTH constant.
As per coding guidelines.
Apply this diff:
@pytest.mark.asyncio
async def test_get_provider_unexpected_exception(mocker):
"""Test that /providers/{provider_id} endpoint raises HTTP 500 for unexpected exceptions."""
mock_client = AsyncMock()
mock_client.providers.list.side_effect = Exception("boom")
mocker.patch(
"app.endpoints.providers.AsyncLlamaStackClientHolder"
).return_value.get_client.return_value = mock_client
request = Request(scope={"type": "http"})
- auth = ("user", "token", {})
+ auth = MOCK_AUTH
with pytest.raises(HTTPException) as e:
await get_provider_endpoint_handler(
request=request, provider_id="openai", auth=auth
)
assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
assert "Unable to retrieve list of providers" in e.value.detail["response"]📝 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.
| @pytest.mark.asyncio | |
| async def test_get_provider_unexpected_exception(mocker): | |
| """Test that /providers/{provider_id} endpoint raises HTTP 500 for unexpected exceptions.""" | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.side_effect = Exception("boom") | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = ("user", "token", {}) | |
| with pytest.raises(HTTPException) as e: | |
| await get_provider_endpoint_handler( | |
| request=request, provider_id="openai", auth=auth | |
| ) | |
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | |
| assert "Unable to retrieve list of providers" in e.value.detail["response"] | |
| @pytest.mark.asyncio | |
| async def test_get_provider_unexpected_exception(mocker): | |
| """Test that /providers/{provider_id} endpoint raises HTTP 500 for unexpected exceptions.""" | |
| mock_client = AsyncMock() | |
| mock_client.providers.list.side_effect = Exception("boom") | |
| mocker.patch( | |
| "app.endpoints.providers.AsyncLlamaStackClientHolder" | |
| ).return_value.get_client.return_value = mock_client | |
| request = Request(scope={"type": "http"}) | |
| auth = MOCK_AUTH | |
| with pytest.raises(HTTPException) as e: | |
| await get_provider_endpoint_handler( | |
| request=request, provider_id="openai", auth=auth | |
| ) | |
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | |
| assert "Unable to retrieve list of providers" in e.value.detail["response"] |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_providers.py around lines 146 to 163, replace
the local auth tuple with the shared MOCK_AUTH constant to ensure consistency
with the test suite; update the test to import or reference MOCK_AUTH (already
available in the test module) and use it in the get_provider_endpoint_handler
call and any related assertions instead of creating a new auth = ("user",
"token", {}) tuple.
src/app/endpoints/providers.py
Outdated
| @@ -0,0 +1,225 @@ | |||
| """Handler for REST API call to list available models.""" | |||
There was a problem hiding this comment.
Code rabbit seems to be right there ;)
tests/unit/app/test_routers.py
Outdated
| assert app.get_router_prefix(models.router) == "/v1" | ||
| assert app.get_router_prefix(tools.router) == "/v1" | ||
| assert app.get_router_prefix(shields.router) == "/v1" | ||
| assert app.get_router_prefix(root.router) == "" |
tests/unit/app/test_routers.py
Outdated
| assert app.get_router_prefix(tools.router) == "/v1" | ||
| assert app.get_router_prefix(shields.router) == "/v1" | ||
| assert app.get_router_prefix(root.router) == "" | ||
| assert app.get_router_prefix(info.router) == "/v1" |
tests/unit/app/test_routers.py
Outdated
| assert app.get_router_prefix(shields.router) == "/v1" | ||
| assert app.get_router_prefix(root.router) == "" | ||
| assert app.get_router_prefix(info.router) == "/v1" | ||
| assert app.get_router_prefix(models.router) == "/v1" |
There was a problem hiding this comment.
Fixed, probably occurred due to a merge with the shields endpoint.
asimurka
force-pushed
the
providers_endpoint
branch
from
f9a977f to
c7fb232
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/endpoints/providers.py (1)
155-225: LGTM!The single provider endpoint handler is well-implemented with proper 404 handling, comprehensive error handling, and correct HTTPException re-raising to avoid masking validation errors.
Optional: On line 196, consider renaming variable
ptoprovidersfor better readability:- p = [dict(p) for p in providers] - match = next((item for item in p if item["provider_id"] == provider_id), None) + providers_list = [dict(p) for p in providers] + match = next((item for item in providers_list if item["provider_id"] == provider_id), None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/auth.md(1 hunks)docs/openapi.json(3 hunks)src/app/endpoints/providers.py(1 hunks)src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(2 hunks)tests/unit/app/endpoints/test_providers.py(1 hunks)tests/unit/app/test_routers.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/models/config.py
- tests/unit/app/endpoints/test_providers.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/unit/app/test_routers.pysrc/app/endpoints/providers.pysrc/app/routers.pysrc/models/responses.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/app/test_routers.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/unit/app/test_routers.py
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/providers.pysrc/app/routers.pysrc/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/providers.pysrc/app/routers.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/providers.pysrc/app/routers.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/providers.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 (2)
src/app/endpoints/providers.py (7)
src/authentication/__init__.py (1)
get_auth_dependency(14-52)src/authorization/middleware.py (1)
authorize(111-122)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/configuration.py (2)
configuration(65-69)llama_stack_configuration(79-83)src/models/config.py (2)
config(139-145)Action(328-371)src/models/responses.py (2)
ProvidersListResponse(89-119)ProviderResponse(122-145)src/utils/endpoints.py (1)
check_configuration_loaded(68-80)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(35-50)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (16)
src/app/routers.py (2)
9-9: LGTM!The providers module import is correctly placed and follows the established import pattern for endpoint modules.
35-35: LGTM!The providers router is correctly included with the appropriate
/v1prefix, consistent with other endpoint routers.docs/auth.md (1)
252-253: LGTM!The new provider-related actions are correctly documented with clear descriptions of their associated endpoints.
tests/unit/app/test_routers.py (3)
16-16: LGTM!The providers import is correctly added alongside other endpoint module imports.
76-76: LGTM!The assertion correctly verifies that the providers router is included in the application.
94-94: LGTM!The router count is correctly updated to 15, and the prefix assertion properly verifies that the providers router uses the
/v1prefix.Also applies to: 100-100
src/models/responses.py (3)
3-3: LGTM!The new imports are correctly added to support the provider response models. Union is needed for complex field types, and ProviderInfo provides the base structure for ProviderResponse.
Also applies to: 7-7
89-119: LGTM!The ProvidersListResponse model is well-defined with appropriate field types and comprehensive examples. The description correctly states "their corresponding providers" (typo was fixed per past review).
122-146: LGTM!The ProviderResponse model appropriately extends ProviderInfo and defines all required fields with flexible types to accommodate various provider configurations and health statuses.
docs/openapi.json (3)
160-212: LGTM!The /v1/providers endpoint definition is complete and correct. The response schema properly references ProvidersListResponse (corrected per past review), and the error handling is appropriate.
214-272: LGTM!The /v1/providers/{provider_id} endpoint definition is comprehensive, with proper parameter definition, appropriate response codes (200, 404, 500), and clear examples for each case.
1233-1234: LGTM!The Action enum additions and schema definitions are correct and complete. The schemas accurately represent the Pydantic models with appropriate field types, descriptions, and examples.
Also applies to: 2694-2792
src/app/endpoints/providers.py (4)
1-21: LGTM!The module imports and setup follow established patterns. The module docstring correctly describes the provider endpoints (corrected per past review), and all necessary dependencies are properly imported.
23-64: LGTM!The response dictionaries provide clear example payloads for both endpoints, covering success and error scenarios. These align with the OpenAPI documentation.
67-131: LGTM!The providers list endpoint handler follows best practices with comprehensive error handling, proper authorization checks (corrected per past review), and clear logging. The client initialization dependency is appropriately handled by the error handling chain.
133-152: LGTM!The group_providers helper function efficiently groups providers by API type using the setdefault pattern. The function assumes valid input from the Llama Stack client, which is reasonable given the defined contract.
tests/unit/app/test_routers.py
Outdated
| assert root.router in app.get_routers() | ||
| assert info.router in app.get_routers() | ||
| assert models.router in app.get_routers() |
There was a problem hiding this comment.
Remove duplicate assertions.
Lines 73-75 duplicate the assertions already made on lines 68-70. These should be removed as they add no value and clutter the test.
Apply this diff to remove the duplicates:
assert tools.router in app.get_routers()
assert shields.router in app.get_routers()
- assert root.router in app.get_routers()
- assert info.router in app.get_routers()
- assert models.router in app.get_routers()
assert providers.router in app.get_routers()
assert query.router in app.get_routers()📝 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.
| assert root.router in app.get_routers() | |
| assert info.router in app.get_routers() | |
| assert models.router in app.get_routers() | |
| assert tools.router in app.get_routers() | |
| assert shields.router in app.get_routers() | |
| assert providers.router in app.get_routers() | |
| assert query.router in app.get_routers() |
🤖 Prompt for AI Agents
In tests/unit/app/test_routers.py around lines 73 to 75, there are duplicate
assertions repeating those at lines 68-70; remove the three redundant lines
"assert root.router in app.get_routers()", "assert info.router in
app.get_routers()", and "assert models.router in app.get_routers()" so the test
only asserts each router once and avoid cluttering the test file.
7602107 to
444e0df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/unit/app/endpoints/test_providers.py (1)
1-14: Use the shared MOCK_AUTH constant across tests.Replace inline tuples with MOCK_AUTH for consistency.
As per coding guidelines.
Minimal patch:
+from tests.constants import MOCK_AUTH ... - auth = ("user", "token", {}) + auth = MOCK_AUTHApply similarly at lines 37, 72, 90, 117, 136, 156.
Also applies to: 20-21, 37-38, 71-73, 89-91, 116-118, 135-137, 155-157
src/app/endpoints/providers.py (1)
1-1: Docstring should cover both list and get endpoints.Update to include “list and retrieve available providers.”
-"""Handler for REST API call to list available providers.""" +"""Handler for REST API calls to list and retrieve available providers."""src/models/responses.py (1)
92-95: Fix typo in description (“thieir” → “their”).Keeps schema/docs clean.
- description="List of available API types and thieir corresponding providers", + description="List of available API types and their corresponding providers",
🧹 Nitpick comments (6)
src/models/responses.py (1)
130-146: Simplify and clarify JSON-like field types (optional).Current union omits int and includes broad object. Prefer a JSON alias or dict[str, Any] for clarity.
Example alias at module level:
JSONValue = Union[None, bool, int, float, str, list["JSONValue"], dict[str, "JSONValue"]]Then:
config: dict[str, JSONValue] health: dict[str, JSONValue]As per coding guidelines.
docs/openapi.json (1)
2880-2925: Align schema title with schema name.Use “ProviderResponse” as the title to reduce confusion.
- "title": "ProviderInfo", + "title": "ProviderResponse",src/app/endpoints/providers.py (4)
7-7: Import Depends from fastapi, not fastapi.params.Keeps imports consistent with app code conventions.
As per coding guidelines.
-from fastapi.params import Depends +from fastapi import Depends
95-97: Avoid logging full Llama Stack configuration at info level.May expose sensitive fields; log minimal, and consider debug level.
Example:
cfg = configuration.llama_stack_configuration logger.debug("Llama stack cfg: url=%s, library=%s", cfg.url, cfg.use_as_library_client)Also applies to: 177-179
128-147: Guard against missing keys in group_providers (optional).Use get() and skip/validate to avoid KeyError if upstream shape changes.
- for provider in providers: - result.setdefault(provider["api"], []).append( - { - "provider_id": provider["provider_id"], - "provider_type": provider["provider_type"], - } - ) + for provider in providers: + api = provider.get("api") + pid = provider.get("provider_id") + ptype = provider.get("provider_type") + if not (api and pid and ptype): + continue + result.setdefault(api, []).append({"provider_id": pid, "provider_type": ptype})
48-64: Align 404 example with actual error detail (optional).The code returns "Provider with id '{provider_id}' not found"; example says "given id". Align wording to reduce confusion.
- 404: {"response": "Provider with given id not found"}, + 404: {"response": "Provider with id '<provider_id>' not found"},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/auth.md(1 hunks)docs/openapi.json(3 hunks)src/app/endpoints/providers.py(1 hunks)src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(2 hunks)tests/unit/app/endpoints/test_providers.py(1 hunks)tests/unit/app/test_routers.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/app/test_routers.py
- src/models/config.py
🧰 Additional context used
📓 Path-based instructions (9)
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/providers.pysrc/app/routers.pysrc/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/providers.pysrc/app/routers.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/app/endpoints/providers.pysrc/app/routers.pytests/unit/app/endpoints/test_providers.pysrc/models/responses.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/providers.pysrc/app/routers.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/providers.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/app/endpoints/test_providers.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/unit/app/endpoints/test_providers.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
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to tests/**/*.py : Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Applied to files:
tests/unit/app/endpoints/test_providers.py
🧬 Code graph analysis (3)
src/app/endpoints/providers.py (7)
src/authentication/__init__.py (1)
get_auth_dependency(14-52)src/authorization/middleware.py (1)
authorize(111-122)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/configuration.py (2)
configuration(65-69)llama_stack_configuration(79-83)src/models/config.py (2)
config(139-145)Action(328-374)src/models/responses.py (2)
ProvidersListResponse(89-119)ProviderResponse(122-145)src/utils/endpoints.py (1)
check_configuration_loaded(68-80)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(35-50)
tests/unit/app/endpoints/test_providers.py (2)
src/app/endpoints/providers.py (2)
get_provider_endpoint_handler(152-215)providers_endpoint_handler(69-125)src/client.py (1)
get_client(49-55)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (2)
src/app/routers.py (1)
9-9: Providers router wired correctly under /v1.Import and include are consistent with existing pattern.
Also applies to: 35-35
docs/auth.md (1)
252-254: Docs updated with new actions look good.Entries align with endpoints.
asimurka
force-pushed
the
providers_endpoint
branch
from
444e0df to
e466c2b
Compare
asimurka
force-pushed
the
providers_endpoint
branch
from
e466c2b to
94ad141
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/models/responses.py (1)
122-146: Consider using modern union syntax and more specific types.Per the coding guidelines, prefer modern union syntax (
|) overUnion. Additionally, usingobjectin the type annotation is overly broad—consider more specific types or justdict[str, Any]if the structure is truly arbitrary.Apply this diff to modernize the type annotations:
- config: dict[str, Union[bool, float, str, list[Any], object, None]] = Field( + config: dict[str, bool | float | str | list[Any] | None] = Field( ..., description="Provider configuration parameters", example={"api_key": "********"}, ) # type: ignore - health: dict[str, Union[bool, float, str, list[Any], object, None]] = Field( + health: dict[str, bool | float | str | list[Any] | None] = Field( ..., description="Current health status of the provider", example={"status": "OK", "message": "Healthy"}, ) # type: ignoreNote: The multiple
# type: ignorecomments suggest that Pydantic might have difficulty with these field overrides when extending ProviderInfo. Consider whether these overrides are necessary or if the base class fields can be used directly.As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/auth.md(1 hunks)docs/openapi.json(3 hunks)src/app/endpoints/providers.py(1 hunks)src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(2 hunks)tests/unit/app/endpoints/test_providers.py(1 hunks)tests/unit/app/test_routers.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/endpoints/providers.py
- tests/unit/app/test_routers.py
- tests/unit/app/endpoints/test_providers.py
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/models/config.pysrc/app/routers.pysrc/models/responses.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/config.pysrc/app/routers.pysrc/models/responses.py
src/{models/config.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields
Files:
src/models/config.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/config.pysrc/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/config.pysrc/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/routers.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/routers.py
🧬 Code graph analysis (1)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(35-50)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (10)
src/models/config.py (1)
367-368: LGTM!The new Action enum members follow the existing naming conventions and align with the new provider endpoints.
src/models/responses.py (3)
3-3: LGTM!Union import added to support type annotations in the new response models.
7-7: LGTM!Importing ProviderInfo from the external llama_stack_client library to extend for the ProviderResponse model.
89-119: LGTM!The ProvidersListResponse model correctly represents a mapping of API types to their providers using a flexible dict structure.
src/app/routers.py (2)
9-9: LGTM!Import added in the correct alphabetical order.
35-35: LGTM!Router correctly wired with the
/v1prefix, consistent with other endpoint routers.docs/openapi.json (3)
206-318: LGTM!The new provider endpoints are well-documented with appropriate response codes, descriptions, and examples. The schema references align with the newly defined models.
1362-1363: LGTM!The Action enum correctly includes the new provider-related actions.
2880-2978: LGTM!The schema definitions for ProviderResponse and ProvidersListResponse are comprehensive, include appropriate examples, and align with the response models defined in the codebase.
docs/auth.md (1)
252-253: LGTM!The new action descriptions are clear, follow the existing format, and correctly document the new provider endpoints.
2 participants
Description
This PR introduces the /providers REST API endpoints, including support for listing all providers and retrieving details of a specific provider. It also adds corresponding response models, error handling, and unit tests to ensure reliability and correctness.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Tests