LCORE-466: Llama Stack version in /info endpoint #469
Conversation
|
Warning Rate limit exceeded@tisnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe /info endpoint now obtains a Llama Stack client and fetches its version asynchronously, updates the response schema to include Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant API as /info Endpoint
participant Holder as AsyncLlamaStackClientHolder
participant LSC as Llama Stack Client
User->>API: GET /info
activate API
API->>Holder: acquire client (async)
Holder-->>API: client
API->>LSC: client.inspect.version() (async)
alt success
LSC-->>API: VersionInfo(version)
API-->>User: 200 OK { name, service_version, llama_stack_version }
else APIConnectionError
LSC-->>API: APIConnectionError
API-->>User: 500 { detail: "Unable to connect to Llama Stack", response, cause }
end
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
tisnik
force-pushed
the
lcore-466-llama-stack-version-in-info-endpoint
branch
from
4f4b6e4 to
9e60ab4
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/openapi.md (1)
36-41: Update /v1/info docs to reflect new fields and add 500 error.The text still says “name and version” and the responses table lacks the documented 500 on Llama connection failures. Update both to avoid drift with implementation/tests.
-Process GET requests to the /info endpoint, returning the -service name and version. +Process GET requests to the /info endpoint, returning the +service name, service version, and Llama Stack version. -Returns: - InfoResponse: An object containing the service's name and version. +Returns: + InfoResponse: An object containing the service's name, service_version, and llama_stack_version.| 200 | Successful Response | [InfoResponse](#inforesponse) | +| 500 | Unable to connect to Llama Stack | [ErrorResponse](#errorresponse) |Also applies to: 48-50
docs/openapi.json (1)
53-68: Fix /v1/info OpenAPI: stale description, invalid extra keys, and missing 500 response.
- Description still mentions just “name and version”.
- The 200 response incorrectly includes top-level “name” and “version” alongside content, which is invalid OpenAPI and also stale.
- Missing 500 response matching the endpoint behavior.
- "description": "Handle request to the /info endpoint.\n\nProcess GET requests to the /info endpoint, returning the\nservice name and version.\n\nReturns:\n InfoResponse: An object containing the service's name and version.", + "description": "Handle request to the /info endpoint.\n\nProcess GET requests to the /info endpoint, returning the service name, service version, and Llama Stack version.\n\nReturns:\n InfoResponse: An object containing the service's name, service_version, and llama_stack_version.", "operationId": "info_endpoint_handler_v1_info_get", "responses": { "200": { "description": "Successful Response", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/InfoResponse" - } + }, + "example": { + "name": "Lightspeed Stack", + "service_version": "1.0.0", + "llama_stack_version": "0.2.18" + } } - }, - "name": "Service name", - "version": "Service version" + } + }, + "500": { + "description": "Unable to connect to Llama Stack", + "content": { + "application/json": { + "schema": { "$ref": "#/components/schemas/ErrorResponse" }, + "example": { + "detail": { + "response": "Unable to connect to Llama Stack", + "cause": "Connection error." + } + } + } + } } }docs/output.md (1)
36-41: Align /v1/info narrative and responses with implementation.Same drift as openapi.md: mention service_version and llama_stack_version, and document 500 response.
-Process GET requests to the /info endpoint, returning the -service name and version. +Process GET requests to the /info endpoint, returning the +service name, service version, and Llama Stack version. -Returns: - InfoResponse: An object containing the service's name and version. +Returns: + InfoResponse: An object containing the service's name, service_version, and llama_stack_version.| 200 | Successful Response | [InfoResponse](#inforesponse) | +| 500 | Unable to connect to Llama Stack | [ErrorResponse](#errorresponse) |Also applies to: 48-50
🧹 Nitpick comments (4)
docs/openapi.json (1)
1609-1637: LGTM on InfoResponse schema; tiny nit on example value.Schema changes are correct. Nit: consider using a realistic Llama Stack semver (e.g., “0.2.18”) in examples for consistency with docs.
Also applies to: 1638-1643
tests/unit/app/endpoints/test_info.py (3)
56-56: Remove duplicate mock_authorization_resolvers call.It’s already invoked on Line 17; the second call is redundant.
- mock_authorization_resolvers(mocker)
50-52: Unnecessary patching of app.endpoints.models.configuration.The handler reads from configuration.configuration; this patch is unused noise. Safe to remove in both tests.
- mock_config = mocker.Mock() - mocker.patch("app.endpoints.models.configuration", mock_config)Also applies to: 107-109
19-41: DRY the repeated test config via a fixture.Extract the shared config_dict/AppConfig init into a pytest fixture to reduce duplication and ease maintenance.
Also applies to: 77-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
docs/openapi.json(1 hunks)docs/openapi.md(1 hunks)docs/output.md(1 hunks)src/app/endpoints/info.py(2 hunks)tests/unit/app/endpoints/test_info.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/endpoints/info.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/app/endpoints/test_info.py (3)
tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/app/endpoints/info.py (1)
info_endpoint_handler(35-74)
🪛 LanguageTool
docs/openapi.md
[grammar] ~898-~898: There might be a mistake here.
Context: ... ``` | Field | Type | Description | |-------|------|-------------| | name | ...
(QB_NEW_EN)
[grammar] ~899-~899: There might be a mistake here.
Context: ...ription | |-------|------|-------------| | name | string | Service name | | servi...
(QB_NEW_EN)
[grammar] ~900-~900: There might be a mistake here.
Context: ...------| | name | string | Service name | | service_version | string | Service ver...
(QB_NEW_EN)
[grammar] ~901-~901: There might be a mistake here.
Context: ...ice_version | string | Service version | | llama_stack_version | string | Llama S...
(QB_NEW_EN)
docs/output.md
[grammar] ~888-~888: There might be a mistake here.
Context: ... ``` | Field | Type | Description | |-------|------|-------------| | name | ...
(QB_NEW_EN)
[grammar] ~889-~889: There might be a mistake here.
Context: ...ription | |-------|------|-------------| | name | string | Service name | | servi...
(QB_NEW_EN)
[grammar] ~890-~890: There might be a mistake here.
Context: ...------| | name | string | Service name | | service_version | string | Service ver...
(QB_NEW_EN)
[grammar] ~891-~891: There might be a mistake here.
Context: ...ice_version | string | Service version | | llama_stack_version | string | Llama S...
(QB_NEW_EN)
⏰ 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
🔇 Additional comments (2)
docs/openapi.md (1)
885-894: LGTM: InfoResponse fields renamed/added correctly.The switch to service_version and addition of llama_stack_version look consistent with code.
Also applies to: 898-903
docs/output.md (1)
875-884: LGTM: InfoResponse docs updated correctly.Fields and example align with the model and tests.
Also applies to: 888-893
| with pytest.raises(HTTPException) as e: | ||
| await info_endpoint_handler(auth=auth, request=request) | ||
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | ||
| assert e.detail["response"] == "Unable to connect to Llama Stack" |
There was a problem hiding this comment.
Bug: exception assertion uses e.detail instead of e.value.detail.
This will fail; ExceptionInfo stores the exception on .value.
with pytest.raises(HTTPException) as e:
await info_endpoint_handler(auth=auth, request=request)
assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
- assert e.detail["response"] == "Unable to connect to Llama Stack"
+ assert e.value.detail["response"] == "Unable to connect to Llama Stack"📝 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.
| with pytest.raises(HTTPException) as e: | |
| await info_endpoint_handler(auth=auth, request=request) | |
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | |
| assert e.detail["response"] == "Unable to connect to Llama Stack" | |
| with pytest.raises(HTTPException) as e: | |
| await info_endpoint_handler(auth=auth, request=request) | |
| assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | |
| assert e.value.detail["response"] == "Unable to connect to Llama Stack" |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_info.py around lines 122 to 125, the test uses
e.detail but ExceptionInfo stores the caught exception on .value; update the
assertions to access the exception via e.value (e.g., e.value.detail) so both
assertions reference e.value (keep the pytest.raises structure and existing
awaits as-is).
1a35054 to
08a4934
Compare
1 participant
Description
LCORE-466: Llama Stack version in
/infoendpointType of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Refactor
Tests