[Draft] LCORE-390: description for more fields #394
Conversation
WalkthroughOpenAPI docs and rendered documentation were enriched with expanded endpoint descriptions, error semantics, examples, and minor schema adjustments; pydantic models in config and responses gained Field metadata for descriptions/examples; duplicate DatabaseConfiguration schema removed; TLS fields marked required in the public schema. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Backend as LlamaStack
Client->>API: GET /v1/conversations/{conversation_id}
API->>API: Validate conversation_id
alt invalid ID
API-->>Client: 400 Bad Request
else valid ID
API->>Backend: Fetch session/chat history
alt backend success
Backend-->>API: Session data
API-->>Client: 200 ConversationResponse
else backend unavailable
API-->>Client: 503 Service Unavailable
end
opt not found
API-->>Client: 404 Not Found
end
end
opt unexpected error
API-->>Client: 500 Internal Server Error
end
sequenceDiagram
participant Client
participant API
participant Backend as LlamaStack
Client->>API: DELETE /v1/conversations/{conversation_id}
API->>API: Validate conversation_id
alt invalid ID
API-->>Client: 400 Bad Request
else valid ID
API->>Backend: Delete session
alt deleted
Backend-->>API: Confirm
API-->>Client: 200 ConversationDeleteResponse
else not found
API-->>Client: 404 Not Found
else backend unavailable
API-->>Client: 503 Service Unavailable
end
end
opt unexpected error
API-->>Client: 500 Internal Server Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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-390-more-fields-descriptions
branch
from
ffc1c85 to
53e4c9f
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: 0
🔭 Outside diff range comments (2)
docs/openapi.json (2)
539-542: Schema/Example mismatch: uses “message” but schema defines “response”.The 200 example for DELETE /v1/conversations/{conversation_id} uses “message”, while ConversationDeleteResponse property is “response”. This will confuse clients and validators.
Apply this diff:
- "message": "Conversation deleted successfully" + "response": "Conversation deleted successfully"
118-118: Move response examples undercontent& remove invalid keysThe OpenAPI spec includes non-standard properties directly under response objects (e.g.
name,version,conversation_id,response,detail, etc.), which violates OAS 3.1’s response schema and will break most tools. All example or custom fields must be placed under:content:
application/json:
examples:
…Endpoints with unexpected response keys:
- GET /v1/info 200: name, version
- GET /v1/models 200: models
- POST /v1/query 200: conversation_id, response
- POST /v1/query 503: detail
- GET /v1/config 200: name, service, llama_stack, user_data_collection, mcp_servers
- GET /v1/config 503: detail
- GET /v1/conversations 200: conversations
- GET /v1/conversations 503: detail
- GET /v1/conversations/{conversation_id} 200: conversation_id, chat_history
- GET /v1/conversations/{conversation_id} 404/503: detail
- DELETE /v1/conversations/{conversation_id} 200: conversation_id, success, message
- DELETE /v1/conversations/{conversation_id} 404/503: detail
Please remove these keys from the response root and relocate any example data into
content -> application/json -> examples(or intoschema.examplesper your preference).
🧹 Nitpick comments (8)
src/models/responses.py (2)
255-264: Nit: Prefer “Username” over “User name” in field description.Use the single-word noun to match common usage and other docs.
Apply this diff:
username: str = Field( ..., - description="User name", + description="Username", examples=["John Doe", "Adam Smith"], )
397-417: Docstring example and attributes are out of sync with the model.The docstring refers to ConversationSummary and “model”, while the class is ConversationDetails with last_used_model/last_used_provider fields.
Consider revising the docstring to reflect the actual fields and class name. I can provide a patch if you’d like to include it in this PR.
src/models/config.py (1)
23-40: Typo: “passowrd” → “password”.Fix the spelling in the TLS key password description to avoid propagating the typo to generated docs.
tls_key_password: Optional[FilePath] = Field( None, - description="Path to file containing TLS key passowrd", + description="Path to file containing TLS key password", examples=["/app-root/certs/password.txt"], )docs/openapi.md (1)
435-437: Minor wording fixes for consistency and polish.
- Prefer “Username” over “User name”:
-| username | string | User name | +| username | string | Username |
- Fix “passowrd” typo:
-| tls_key_password | | Path to file containing TLS key passowrd | +| tls_key_password | | Path to file containing TLS key password |Also applies to: 1017-1020
docs/openapi.json (2)
839-845: Nit: Use “Username” in description.- "description": "User name", + "description": "Username",
2054-2058: Typo in TLS description: “passowrd” → “password”.- "description": "Path to file containing TLS key passowrd", + "description": "Path to file containing TLS key password",docs/output.md (2)
179-183: Minor language fixes (“cannot”, “Username”, and typo).
- Prefer “cannot”:
-| 500 | User feedback can not be stored | [ErrorResponse](#errorresponse) | +| 500 | User feedback cannot be stored | [ErrorResponse](#errorresponse) |
- Prefer “Username” over “User name”:
-| username | string | User name | +| username | string | Username |
- Fix “passowrd” typo:
-| tls_key_password | | Path to file containing TLS key passowrd | +| tls_key_password | | Path to file containing TLS key password |Also applies to: 435-437, 1005-1008
485-506: Inconsistency: ConversationDetails attributes mention “model” but fields list last_used_model/last_used_provider.Align the “Attributes” prose and examples with the fields table that already lists last_used_model and last_used_provider.
I can generate a patch to update the prose and example blocks accordingly—say the word.
Also applies to: 509-517
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/openapi.json(13 hunks)docs/openapi.md(9 hunks)docs/output.md(18 hunks)src/models/config.py(3 hunks)src/models/responses.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pyright
src/models/config.py
[error] 128-128: uv run pyright src: Pyright error: Missing arguments for parameters tls_certificate_path, tls_key_path, tls_key_password (reportCallIssue).
[error] 338-338: uv run pyright src: Pyright error: Missing arguments for parameters module, skip_tls_verification, k8s_cluster_api, k8s_ca_cert_path, jwk_config (reportCallIssue).
🪛 GitHub Actions: Type checks
src/models/config.py
[error] 128-128: Mypy error while running command 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/': Missing named argument "tls_certificate_path" for "TLSConfiguration" [call-arg]
[error] 128-128: Mypy error while running command 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/': Missing named argument "tls_key_path" for "TLSConfiguration" [call-arg]
[error] 128-128: Mypy error while running command 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/': Missing named argument "tls_key_password" for "TLSConfiguration" [call-arg]
[error] 339-339: Mypy error while running command 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/': Missing named argument "module" for "AuthenticationConfiguration" [call-arg]
[error] 339-339: Mypy error while running command 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/': Missing named argument "skip_tls_verification" for "AuthenticationConfiguration" [call-arg]
[error] 339-339: Mypy error while running command 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/': Missing named argument "k8s_cluster_api" for "AuthenticationConfiguration" [call-arg]
[error] 339-339: Mypy error while running command 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/': Missing named argument "k8s_ca_cert_path" for "AuthenticationConfiguration" [call-arg]
[error] 339-339: Mypy error while running command 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/': Missing named argument "jwk_config" for "AuthenticationConfiguration" [call-arg]
🪛 LanguageTool
docs/openapi.md
[style] ~436-~436: It’s more common nowadays to write this noun as one word.
Context: ...or example UUID | | username | string | User name | ## Configuration Global service c...
(RECOMMENDED_COMPOUNDS)
docs/output.md
[style] ~182-~182: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...biddenresponse) | | 500 | User feedback can not be stored | [ErrorResponse](#errorrespo...
(CAN_NOT_PREMIUM)
[style] ~436-~436: It’s more common nowadays to write this noun as one word.
Context: ...or example UUID | | username | string | User name | ## Configuration Global service c...
(RECOMMENDED_COMPOUNDS)
🔇 Additional comments (6)
src/models/responses.py (1)
5-5: Import of Field is correct and consistent with Pydantic v2 usage.No issues here.
src/models/config.py (3)
6-12: LGTM on importing Field and typed helpers.Consistent with the metadata additions elsewhere.
233-261: Field metadata additions look good.Clear descriptions and examples for AuthenticationConfiguration fields; no functional changes to validators.
122-129: Manual Verification Required: Ensure Pyright/Mypy Pass withdefault_factoryChangesI wasn’t able to run Pyright or Mypy in this environment—please apply the suggested diff to replace direct instantiations and mutable defaults with
Field(default_factory=…), then verify locally that all static-analysis errors are resolved.Key changes to apply:
In
src/models/config.py(around lines 122–129):
• Replacetls_config: TLSConfiguration = TLSConfiguration()with
tls_config: TLSConfiguration = Field(default_factory=TLSConfiguration)In the same file (around lines 332–343):
• Changedatabase: DatabaseConfiguration = DatabaseConfiguration() mcp_servers: list[ModelContextProtocolServer] = [] authentication: Optional[AuthenticationConfiguration] = ( AuthenticationConfiguration() ) inference: InferenceConfiguration = InferenceConfiguration()to
database: DatabaseConfiguration = Field(default_factory=DatabaseConfiguration) mcp_servers: list[ModelContextProtocolServer] = Field(default_factory=list) authentication: Optional[AuthenticationConfiguration] = Field( default_factory=AuthenticationConfiguration ) inference: InferenceConfiguration = Field(default_factory=InferenceConfiguration)Once applied, run your local Pyright and Mypy checks to confirm that:
- Missing-args errors for
TLSConfiguration()andAuthenticationConfiguration()are gone.- Shared mutable default (the list) is replaced by a safe factory.
docs/openapi.md (1)
59-68: LGTM on expanded endpoint prose and return types.The added descriptions, error semantics, and return type references improve API clarity and align with code.
Also applies to: 66-68, 98-103, 151-166, 190-195, 227-240, 264-271
docs/output.md (1)
59-68: LGTM on expanded documentation and response tables.The updates improve clarity and align with schema changes.
Also applies to: 98-103, 151-166, 190-195, 227-240, 264-271, 285-289, 307-309, 326-327, 348-350
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
docs/openapi.json (2)
517-542: Inconsistent property name in DELETE conversation example: use 'response' instead of 'message'The schema ConversationDeleteResponse uses 'response', but the 200 response example uses 'message'.
- "success": true, - "message": "Conversation deleted successfully" + "success": true, + "response": "Conversation deleted successfully"
2022-2066: TLSConfiguration: remove required fields and fix “passowrd” typo to match Optional model defaultsThe model fields are Optional[FilePath] and tests allow None; making them required in the public schema conflicts with defaults (and with ServiceConfiguration.tls_config default {}). Also fix the typo.
- "title": "Tls Key Password", - "description": "Path to file containing TLS key passowrd", + "title": "Tls Key Password", + "description": "Path to file containing TLS key password", @@ - "required": [ - "tls_certificate_path", - "tls_key_path", - "tls_key_password" - ], + "required": [],Optionally, if you prefer keeping "required" absent (more idiomatic), remove the key entirely:
- "required": [],
🧹 Nitpick comments (4)
docs/output.md (2)
459-461: Use “Username” instead of “User name”Minor copy edit to common compound noun form.
-| username | string | User name | +| username | string | Username |
1029-1032: Fix typo: “passowrd” → “password”Typo in TLSConfiguration field description.
-| tls_key_password | | Path to file containing TLS key passowrd | +| tls_key_password | | Path to file containing TLS key password |docs/openapi.json (2)
831-845: Use “Username” instead of “User name” in AuthorizedResponse descriptionAligns with common usage and the docs/output.md edit.
- "description": "User name", + "description": "Username",
1091-1109: Example still uses legacy 'model' field; update to 'last_used_model' and add 'last_used_provider'The schema defines last_used_model/last_used_provider. Update example entries accordingly.
- "message_count": 5, - "model": "gemini/gemini-2.0-flash" + "message_count": 5, + "last_used_model": "gemini/gemini-2.0-flash", + "last_used_provider": "gemini" @@ - "message_count": 2, - "model": "gemini/gemini-2.5-flash" + "message_count": 2, + "last_used_model": "gemini/gemini-2.5-flash", + "last_used_provider": "gemini"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/openapi.json(13 hunks)docs/output.md(9 hunks)src/models/config.py(5 hunks)tests/unit/models/test_config.py(4 hunks)tests/unit/runners/test_uvicorn_runner.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/models/config.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/unit/runners/test_uvicorn_runner.py (1)
src/models/config.py (1)
TLSConfiguration(20-41)
tests/unit/models/test_config.py (1)
src/models/config.py (2)
TLSConfiguration(20-41)ServiceConfiguration(116-140)
🪛 GitHub Actions: Python linter
tests/unit/models/test_config.py
[error] 243-243: Pylint E1101: FieldInfo has no 'tls_certificate_path' member (no-member).
[error] 244-244: Pylint E1101: FieldInfo has no 'tls_key_path' member (no-member).
[error] 245-245: Pylint E1101: FieldInfo has no 'tls_key_password' member (no-member).
🪛 LanguageTool
docs/output.md
[style] ~460-~460: It’s more common nowadays to write this noun as one word.
Context: ...or example UUID | | username | string | User name | ## Configuration Global service c...
(RECOMMENDED_COMPOUNDS)
⏰ 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: e2e_tests
- GitHub Check: build-pr
🔇 Additional comments (9)
tests/unit/runners/test_uvicorn_runner.py (2)
55-57: LGTM: explicit empty TLSConfiguration is consistent with model defaultsConstructing TLSConfiguration with explicit Nones matches the updated model metadata and keeps intent clear in the test.
99-101: Double-check: uvicorn expects the key passphrase value, not a filesystem pathThe test asserts that ssl_keyfile_password equals a path string. Uvicorn’s ssl_keyfile_password parameter expects the passphrase itself. If start_uvicorn reads the file and passes the content, the test should assert the content. If it passes the path, consider fixing the implementation.
Suggested test update (if implementation reads the file content):
@@ with patch("uvicorn.run") as mocked_run: start_uvicorn(configuration) - mocked_run.assert_called_once_with( + pwd = Path("tests/configuration/password").read_text(encoding="utf-8").strip() + mocked_run.assert_called_once_with( "app.main:app", host="x.y.com", port=1234, workers=10, log_level=20, ssl_certfile=Path("tests/configuration/server.crt"), ssl_keyfile=Path("tests/configuration/server.key"), - ssl_keyfile_password="tests/configuration/password", + ssl_keyfile_password=pwd, use_colors=True, access_log=True, )Do you want me to scan start_uvicorn to confirm whether we pass the file content or the path?
tests/unit/models/test_config.py (3)
45-47: LGTM: default TLS config assertion updatedThe assertion matches the new default TLSConfiguration(None, None, None) as reflected in ServiceConfiguration.
438-443: LGTM: dump includes TLS config under serviceThe dump configuration now correctly serializes TLS fields under service.tls_config.
489-492: LGTM: expected JSON updated for new TLS fieldsThe expected content reflects tls_certificate_path, tls_key_path, and tls_key_password in the service dump.
docs/output.md (3)
59-68: Docs enhancement for /v1/models reads clearlyThe added behavior/semantics are clear and helpful.
157-172: Docs enhancement for /v1/feedback is clearDescribes flow, dependencies, and error semantics concisely.
201-206: Docs enhancement for feedback status is preciseReturn type and meaning are clear.
docs/openapi.json (1)
1973-1976: ServiceConfiguration.tls_config default{}may violate TLSConfiguration “required” listIf you decide to keep TLS fields required, the tls_config default should include the keys with null values; otherwise, drop “required” as suggested above.
- "tls_config": { - "$ref": "#/components/schemas/TLSConfiguration", - "default": {} - } + "tls_config": { + "$ref": "#/components/schemas/TLSConfiguration", + "default": { + "tls_certificate_path": null, + "tls_key_path": null, + "tls_key_password": null + } + }Would you like me to regenerate the OpenAPI after adjusting the models so this stays consistent?
| def test_tls_configuration_in_service_configuration() -> None: | ||
| """Test the TLS configuration in service configuration.""" | ||
| cfg = ServiceConfiguration( | ||
| tls_config=TLSConfiguration( | ||
| tls_certificate_path=Path("tests/configuration/server.crt"), | ||
| tls_key_path=Path("tests/configuration/server.key"), | ||
| tls_key_password=Path("tests/configuration/password"), | ||
| ) | ||
| ) | ||
| assert cfg is not None | ||
| assert cfg.tls_config is not None | ||
| assert cfg.tls_config.tls_certificate_path == Path("tests/configuration/server.crt") | ||
| assert cfg.tls_config.tls_key_path == Path("tests/configuration/server.key") | ||
| assert cfg.tls_config.tls_key_password == Path("tests/configuration/password") | ||
|
|
There was a problem hiding this comment.
Pylint E1101 in CI: tls_config inferred as FieldInfo; fix model default, or locally silence in test
CI shows Pylint E1101 “FieldInfo has no 'tls_*' member” on these asserts (Lines 243–245). This is due to ServiceConfiguration.tls_config being defined via Field(...), which leads pylint to infer FieldInfo as the attribute type.
Recommended fix in src/models/config.py (aligns with pydantic v2 best practices and avoids pylint false-positives):
- Give explicit None defaults for TLSConfiguration fields.
- Avoid Field() for tls_config default and use a plain default instance (or a default_factory).
Example update (outside this file):
# src/models/config.py
class TLSConfiguration(BaseModel):
tls_certificate_path: Optional[FilePath] = Field(
None, description="Path to TLS certificate", examples=["/etc/certs/certs.pem"]
)
tls_key_path: Optional[FilePath] = Field(
None, description="Path to TLS certificate key", examples=["/etc/certs/key.pem"]
)
tls_key_password: Optional[FilePath] = Field(
None, description="Path to file containing TLS key password", examples=["/app-root/certs/password.txt"]
)
class ServiceConfiguration(BaseModel):
...
# Option A (simplest, best for linting and readability)
tls_config: TLSConfiguration = TLSConfiguration(
tls_certificate_path=None, tls_key_path=None, tls_key_password=None
)
# Option B (also fine)
# tls_config: TLSConfiguration = Field(
# default_factory=lambda: TLSConfiguration(
# tls_certificate_path=None, tls_key_path=None, tls_key_password=None
# )
# )If changing the model is out-of-scope for this PR, locally silence pylint for the dynamically provided pydantic attributes:
@@ def test_tls_configuration_in_service_configuration() -> None:
- assert cfg.tls_config.tls_certificate_path == Path("tests/configuration/server.crt")
- assert cfg.tls_config.tls_key_path == Path("tests/configuration/server.key")
- assert cfg.tls_config.tls_key_password == Path("tests/configuration/password")
+ # pylint: disable=no-member
+ assert cfg.tls_config.tls_certificate_path == Path("tests/configuration/server.crt")
+ assert cfg.tls_config.tls_key_path == Path("tests/configuration/server.key")
+ assert cfg.tls_config.tls_key_password == Path("tests/configuration/password")I can open a follow-up PR to adjust src/models/config.py and regenerate the schema to keep CI green. Prefer Option A?
🧰 Tools
🪛 GitHub Actions: Python linter
[error] 243-243: Pylint E1101: FieldInfo has no 'tls_certificate_path' member (no-member).
[error] 244-244: Pylint E1101: FieldInfo has no 'tls_key_path' member (no-member).
[error] 245-245: Pylint E1101: FieldInfo has no 'tls_key_password' member (no-member).
🤖 Prompt for AI Agents
In tests/unit/models/test_config.py around lines 232–246, pylint E1101 is raised
because ServiceConfiguration.tls_config is defined using Field(...) so static
analysis infers FieldInfo without tls_* members; fix src/models/config.py by
making TLSConfiguration fields explicitly Optional with default None and change
ServiceConfiguration.tls_config to have a concrete default (either a
TLSConfiguration(...) instance with None values or use a default_factory that
constructs one) so attributes are statically visible to linters; if changing the
model is out of scope, alternatively add a local pylint silence for E1101 in
this test file only.
1 participant
Description
LCORE-390: description for more fields
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Documentation
Bug Fixes